From 7efb2d473868d89f840687f958bf8d7f613446b0 Mon Sep 17 00:00:00 2001 From: Tim Hockin Date: Sun, 25 Sep 2016 22:32:59 -0700 Subject: [PATCH] Always emit autoConvert funcs, but call for help Previously we refused to emit 'autoConvert_*' functions if any field was not convertible. The way around this was to write manual Conversion functions, but to do so safely you must handle every fields. Huge opportunity for errors. This PR cleans up the filtering such that it only operates on types that should be converted (remove a lot of code) and tracks when fields are skipped. In that case, it emits an 'autoConvert' function but not a public 'Convert' function. If there is no manual function, the compile will fail. This also means that manual conversion functions can call autoConvert functions and then "patch up" what they need. --- .../conversion-gen/generators/conversion.go | 158 +++++------------- .../apis/core/v1/zz_generated.conversion.go | 35 ++++ .../apps/v1alpha1/zz_generated.conversion.go | 45 +++++ .../v1beta1/zz_generated.conversion.go | 74 ++++++++ 4 files changed, 192 insertions(+), 120 deletions(-) create mode 100644 federation/apis/core/v1/zz_generated.conversion.go diff --git a/cmd/libs/go2idl/conversion-gen/generators/conversion.go b/cmd/libs/go2idl/conversion-gen/generators/conversion.go index e4c0af9d87b..210f20725bb 100644 --- a/cmd/libs/go2idl/conversion-gen/generators/conversion.go +++ b/cmd/libs/go2idl/conversion-gen/generators/conversion.go @@ -284,87 +284,14 @@ func findMember(t *types.Type, name string) (types.Member, bool) { return types.Member{}, false } -func isConvertible(in, out *types.Type, manualConversions conversionFuncMap) bool { - // If there is pre-existing conversion function, return true immediately. - if _, ok := manualConversions[conversionPair{in, out}]; ok { - return true - } - return isDirectlyConvertible(in, out, manualConversions) -} - -func isDirectlyConvertible(in, out *types.Type, manualConversions conversionFuncMap) bool { - // If one of the types is Alias, resolve it. - if in.Kind == types.Alias { - return isConvertible(in.Underlying, out, manualConversions) - } - if out.Kind == types.Alias { - return isConvertible(in, out.Underlying, manualConversions) - } - - if in == out { - return true - } - if in.Kind != out.Kind { - return false - } - switch in.Kind { - case types.Builtin, types.Struct, types.Map, types.Slice, types.Pointer: - default: - // We don't support conversion of other types yet. - return false - } - - switch in.Kind { - case types.Builtin: - // TODO: Support more conversion types. - return types.IsInteger(in) && types.IsInteger(out) - case types.Struct: - convertible := true - for _, inMember := range in.Members { - // Check if this member is excluded from conversion - if tagvals := extractTag(inMember.CommentLines); tagvals != nil && tagvals[0] == "false" { - continue - } - // Check if there is an out member with that name. - outMember, found := findMember(out, inMember.Name) - if !found { - glog.V(5).Infof("%s is not directly convertible to %s because the destination field %q did not exist", in.Name, out.Name, inMember.Name) - return false - } - convertible = convertible && isConvertible(inMember.Type, outMember.Type, manualConversions) - } - return convertible - case types.Map: - return isConvertible(in.Key, out.Key, manualConversions) && isConvertible(in.Elem, out.Elem, manualConversions) - case types.Slice: - return isConvertible(in.Elem, out.Elem, manualConversions) - case types.Pointer: - return isConvertible(in.Elem, out.Elem, manualConversions) - } - glog.Fatalf("All other types should be filtered before") - return false -} - // unwrapAlias recurses down aliased types to find the bedrock type. func unwrapAlias(in *types.Type) *types.Type { - if in.Kind == types.Alias { - return unwrapAlias(in.Underlying) + for in.Kind == types.Alias { + in = in.Underlying } return in } -func areTypesAliased(in, out *types.Type) bool { - // If one of the types is Alias, resolve it. - if in.Kind == types.Alias { - return areTypesAliased(in.Underlying, out) - } - if out.Kind == types.Alias { - return areTypesAliased(in, out.Underlying) - } - - return in == out -} - const ( runtimePackagePath = "k8s.io/kubernetes/pkg/runtime" conversionPackagePath = "k8s.io/kubernetes/pkg/conversion" @@ -378,7 +305,8 @@ type genConversion struct { manualConversions conversionFuncMap manualDefaulters defaulterFuncMap imports namer.ImportTracker - typesForInit []conversionPair + types []*types.Type + skippedFields map[*types.Type][]string } func NewGenConversion(sanitizedName, targetPackage string, manualConversions conversionFuncMap, manualDefaulters defaulterFuncMap, peerPkgs []string) generator.Generator { @@ -391,7 +319,8 @@ func NewGenConversion(sanitizedName, targetPackage string, manualConversions con manualConversions: manualConversions, manualDefaulters: manualDefaulters, imports: generator.NewImportTracker(), - typesForInit: make([]conversionPair, 0), + types: []*types.Type{}, + skippedFields: map[*types.Type][]string{}, } } @@ -456,18 +385,9 @@ func (g *genConversion) Filter(c *generator.Context, t *types.Type) bool { if !g.convertibleOnlyWithinPackage(t, peerType) { return false } - // We explicitly return true if any conversion is possible - this needs - // to be checked again while generating code for that type. - convertible := false - if isConvertible(t, peerType, g.manualConversions) { - g.typesForInit = append(g.typesForInit, conversionPair{t, peerType}) - convertible = true - } - if isConvertible(peerType, t, g.manualConversions) { - g.typesForInit = append(g.typesForInit, conversionPair{peerType, t}) - convertible = true - } - return convertible + + g.types = append(g.types, t) + return true } func (g *genConversion) isOtherPackage(pkg string) bool { @@ -525,8 +445,10 @@ func (g *genConversion) Init(c *generator.Context, w io.Writer) error { sw.Do("// Public to allow building arbitrary schemes.\n", nil) sw.Do("func RegisterConversions(scheme $.|raw$) error {\n", schemePtr) sw.Do("return scheme.AddGeneratedConversionFuncs(\n", nil) - for _, conv := range g.typesForInit { - sw.Do(nameTmpl+",\n", argsFromType(conv.inType, conv.outType)) + for _, t := range g.types { + peerType := getPeerTypeFor(c, t, g.peerPackages) + sw.Do(nameTmpl+",\n", argsFromType(t, peerType)) + sw.Do(nameTmpl+",\n", argsFromType(peerType, t)) } sw.Do(")\n", nil) sw.Do("}\n\n", nil) @@ -535,33 +457,10 @@ func (g *genConversion) Init(c *generator.Context, w io.Writer) error { func (g *genConversion) GenerateType(c *generator.Context, t *types.Type, w io.Writer) error { glog.V(5).Infof("generating for type %v", t) - sw := generator.NewSnippetWriter(w, c, "$", "$") peerType := getPeerTypeFor(c, t, g.peerPackages) - didForward, didBackward := false, false - if isDirectlyConvertible(t, peerType, g.manualConversions) { - didForward = true - g.generateConversion(t, peerType, sw) - } - if isDirectlyConvertible(peerType, t, g.manualConversions) { - didBackward = true - g.generateConversion(peerType, t, sw) - } - if didForward != didBackward { - if didForward { - glog.Fatalf("Could only generate one direction of conversion for %v -> %v", t, peerType) - } else { - glog.Fatalf("Could only generate one direction of conversion for %v <- %v", t, peerType) - } - } - if !didForward && !didBackward { - // TODO: This should be fatal but we have at least 8 types that - // currently fail this. The right thing to do is to figure out why they - // can't be generated and mark those fields as - // +k8s:conversion-gen=false, and ONLY do manual conversions for those - // fields, with the manual Convert_...() calling autoConvert_...() - // first. - glog.Errorf("Warning: could not generate autoConvert functions for %v <-> %v", t, peerType) - } + sw := generator.NewSnippetWriter(w, c, "$", "$") + g.generateConversion(t, peerType, sw) + g.generateConversion(peerType, t, sw) return sw.Error() } @@ -578,8 +477,17 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene sw.Do("return nil\n", nil) sw.Do("}\n\n", nil) - // If there is no public manual Conversion method, generate it. - if _, ok := g.preexists(inType, outType); !ok { + if _, found := g.preexists(inType, outType); found { + // There is a public manual Conversion method: use it. + } else if skipped := g.skippedFields[inType]; len(skipped) != 0 { + // The inType had some fields we could not generate. + glog.Errorf("Warning: could not find nor generate a final Conversion function for %v -> %v", inType, outType) + glog.Errorf(" the following fields need manual conversion:") + for _, f := range skipped { + glog.Errorf(" - %v", f) + } + } else { + // Emit a public conversion function. sw.Do("func "+nameTmpl+"(in *$.inType|raw$, out *$.outType|raw$, s $.Scope|raw$) error {\n", args) sw.Do("return auto"+nameTmpl+"(in, out, s)\n", args) sw.Do("}\n\n", nil) @@ -590,6 +498,7 @@ func (g *genConversion) generateConversion(inType, outType *types.Type, sw *gene // at any nesting level. This makes the autogenerator easy to understand, and // the compiler shouldn't care. func (g *genConversion) generateFor(inType, outType *types.Type, sw *generator.SnippetWriter) { + glog.V(5).Infof("generating %v -> %v", inType, outType) var f func(*types.Type, *types.Type, *generator.SnippetWriter) switch inType.Kind { case types.Builtin: @@ -717,6 +626,7 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip copied.Name = outMemberType.Name outMemberType = &copied } + args := map[string]interface{}{ "inType": inMemberType, "outType": outMemberType, @@ -730,6 +640,14 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip sw.Do("}\n", nil) continue } + + // If we can't auto-convert, punt before we emit any code. + if inMemberType.Kind != outMemberType.Kind { + sw.Do("// WARNING: field '"+inMember.Name+"' requires manual conversion\n", nil) + g.skippedFields[inType] = append(g.skippedFields[inType], inMember.Name) + continue + } + switch inMemberType.Kind { case types.Builtin: if inMemberType == outMemberType { @@ -793,7 +711,7 @@ func (g *genConversion) doStruct(inType, outType *types.Type, sw *generator.Snip } func (g *genConversion) isDirectlyAssignable(inType, outType *types.Type) bool { - return inType == outType || areTypesAliased(inType, outType) + return unwrapAlias(inType) == unwrapAlias(outType) } func (g *genConversion) doPointer(inType, outType *types.Type, sw *generator.SnippetWriter) { diff --git a/federation/apis/core/v1/zz_generated.conversion.go b/federation/apis/core/v1/zz_generated.conversion.go new file mode 100644 index 00000000000..24cdcb382e5 --- /dev/null +++ b/federation/apis/core/v1/zz_generated.conversion.go @@ -0,0 +1,35 @@ +// +build !ignore_autogenerated + +/* +Copyright 2016 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +// This file was autogenerated by conversion-gen. Do not edit it manually! + +package v1 + +import ( + runtime "k8s.io/kubernetes/pkg/runtime" +) + +func init() { + SchemeBuilder.Register(RegisterConversions) +} + +// RegisterConversions adds conversion functions to the given scheme. +// Public to allow building arbitrary schemes. +func RegisterConversions(scheme *runtime.Scheme) error { + return scheme.AddGeneratedConversionFuncs() +} diff --git a/pkg/apis/apps/v1alpha1/zz_generated.conversion.go b/pkg/apis/apps/v1alpha1/zz_generated.conversion.go index 767eff72862..c01ce9508c4 100644 --- a/pkg/apis/apps/v1alpha1/zz_generated.conversion.go +++ b/pkg/apis/apps/v1alpha1/zz_generated.conversion.go @@ -22,6 +22,7 @@ package v1alpha1 import ( api "k8s.io/kubernetes/pkg/api" + v1 "k8s.io/kubernetes/pkg/api/v1" apps "k8s.io/kubernetes/pkg/apis/apps" conversion "k8s.io/kubernetes/pkg/conversion" runtime "k8s.io/kubernetes/pkg/runtime" @@ -139,6 +140,50 @@ func Convert_apps_PetSetList_To_v1alpha1_PetSetList(in *apps.PetSetList, out *Pe return autoConvert_apps_PetSetList_To_v1alpha1_PetSetList(in, out, s) } +func autoConvert_v1alpha1_PetSetSpec_To_apps_PetSetSpec(in *PetSetSpec, out *apps.PetSetSpec, s conversion.Scope) error { + // WARNING: field 'Replicas' requires manual conversion + out.Selector = in.Selector + if err := v1.Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(&in.Template, &out.Template, s); err != nil { + return err + } + if in.VolumeClaimTemplates != nil { + in, out := &in.VolumeClaimTemplates, &out.VolumeClaimTemplates + *out = make([]api.PersistentVolumeClaim, len(*in)) + for i := range *in { + // TODO: Inefficient conversion - can we improve it? + if err := s.Convert(&(*in)[i], &(*out)[i], 0); err != nil { + return err + } + } + } else { + out.VolumeClaimTemplates = nil + } + out.ServiceName = in.ServiceName + return nil +} + +func autoConvert_apps_PetSetSpec_To_v1alpha1_PetSetSpec(in *apps.PetSetSpec, out *PetSetSpec, s conversion.Scope) error { + // WARNING: field 'Replicas' requires manual conversion + out.Selector = in.Selector + if err := v1.Convert_api_PodTemplateSpec_To_v1_PodTemplateSpec(&in.Template, &out.Template, s); err != nil { + return err + } + if in.VolumeClaimTemplates != nil { + in, out := &in.VolumeClaimTemplates, &out.VolumeClaimTemplates + *out = make([]v1.PersistentVolumeClaim, len(*in)) + for i := range *in { + // TODO: Inefficient conversion - can we improve it? + if err := s.Convert(&(*in)[i], &(*out)[i], 0); err != nil { + return err + } + } + } else { + out.VolumeClaimTemplates = nil + } + out.ServiceName = in.ServiceName + return nil +} + func autoConvert_v1alpha1_PetSetStatus_To_apps_PetSetStatus(in *PetSetStatus, out *apps.PetSetStatus, s conversion.Scope) error { out.ObservedGeneration = in.ObservedGeneration out.Replicas = int(in.Replicas) diff --git a/pkg/apis/extensions/v1beta1/zz_generated.conversion.go b/pkg/apis/extensions/v1beta1/zz_generated.conversion.go index 70d33c1029c..9eac33030c5 100644 --- a/pkg/apis/extensions/v1beta1/zz_generated.conversion.go +++ b/pkg/apis/extensions/v1beta1/zz_generated.conversion.go @@ -941,6 +941,18 @@ func Convert_autoscaling_HorizontalPodAutoscalerList_To_v1beta1_HorizontalPodAut return autoConvert_autoscaling_HorizontalPodAutoscalerList_To_v1beta1_HorizontalPodAutoscalerList(in, out, s) } +func autoConvert_v1beta1_HorizontalPodAutoscalerSpec_To_autoscaling_HorizontalPodAutoscalerSpec(in *HorizontalPodAutoscalerSpec, out *autoscaling.HorizontalPodAutoscalerSpec, s conversion.Scope) error { + out.MinReplicas = in.MinReplicas + out.MaxReplicas = in.MaxReplicas + return nil +} + +func autoConvert_autoscaling_HorizontalPodAutoscalerSpec_To_v1beta1_HorizontalPodAutoscalerSpec(in *autoscaling.HorizontalPodAutoscalerSpec, out *HorizontalPodAutoscalerSpec, s conversion.Scope) error { + out.MinReplicas = in.MinReplicas + out.MaxReplicas = in.MaxReplicas + return nil +} + func autoConvert_v1beta1_HorizontalPodAutoscalerStatus_To_autoscaling_HorizontalPodAutoscalerStatus(in *HorizontalPodAutoscalerStatus, out *autoscaling.HorizontalPodAutoscalerStatus, s conversion.Scope) error { out.ObservedGeneration = in.ObservedGeneration out.LastScaleTime = in.LastScaleTime @@ -1432,6 +1444,44 @@ func Convert_batch_JobList_To_v1beta1_JobList(in *batch.JobList, out *JobList, s return autoConvert_batch_JobList_To_v1beta1_JobList(in, out, s) } +func autoConvert_v1beta1_JobSpec_To_batch_JobSpec(in *JobSpec, out *batch.JobSpec, s conversion.Scope) error { + out.Parallelism = in.Parallelism + out.Completions = in.Completions + out.ActiveDeadlineSeconds = in.ActiveDeadlineSeconds + if in.Selector != nil { + in, out := &in.Selector, &out.Selector + *out = new(unversioned.LabelSelector) + if err := Convert_v1beta1_LabelSelector_To_unversioned_LabelSelector(*in, *out, s); err != nil { + return err + } + } else { + out.Selector = nil + } + if err := v1.Convert_v1_PodTemplateSpec_To_api_PodTemplateSpec(&in.Template, &out.Template, s); err != nil { + return err + } + return nil +} + +func autoConvert_batch_JobSpec_To_v1beta1_JobSpec(in *batch.JobSpec, out *JobSpec, s conversion.Scope) error { + out.Parallelism = in.Parallelism + out.Completions = in.Completions + out.ActiveDeadlineSeconds = in.ActiveDeadlineSeconds + if in.Selector != nil { + in, out := &in.Selector, &out.Selector + *out = new(LabelSelector) + if err := Convert_unversioned_LabelSelector_To_v1beta1_LabelSelector(*in, *out, s); err != nil { + return err + } + } else { + out.Selector = nil + } + if err := v1.Convert_api_PodTemplateSpec_To_v1_PodTemplateSpec(&in.Template, &out.Template, s); err != nil { + return err + } + return nil +} + func autoConvert_v1beta1_JobStatus_To_batch_JobStatus(in *JobStatus, out *batch.JobStatus, s conversion.Scope) error { if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions @@ -2282,6 +2332,18 @@ func Convert_extensions_RollbackConfig_To_v1beta1_RollbackConfig(in *extensions. return autoConvert_extensions_RollbackConfig_To_v1beta1_RollbackConfig(in, out, s) } +func autoConvert_v1beta1_RollingUpdateDeployment_To_extensions_RollingUpdateDeployment(in *RollingUpdateDeployment, out *extensions.RollingUpdateDeployment, s conversion.Scope) error { + // WARNING: field 'MaxUnavailable' requires manual conversion + // WARNING: field 'MaxSurge' requires manual conversion + return nil +} + +func autoConvert_extensions_RollingUpdateDeployment_To_v1beta1_RollingUpdateDeployment(in *extensions.RollingUpdateDeployment, out *RollingUpdateDeployment, s conversion.Scope) error { + // WARNING: field 'MaxUnavailable' requires manual conversion + // WARNING: field 'MaxSurge' requires manual conversion + return nil +} + func autoConvert_v1beta1_RunAsUserStrategyOptions_To_extensions_RunAsUserStrategyOptions(in *RunAsUserStrategyOptions, out *extensions.RunAsUserStrategyOptions, s conversion.Scope) error { out.Rule = extensions.RunAsUserStrategy(in.Rule) if in.Ranges != nil { @@ -2420,6 +2482,18 @@ func Convert_extensions_ScaleSpec_To_v1beta1_ScaleSpec(in *extensions.ScaleSpec, return autoConvert_extensions_ScaleSpec_To_v1beta1_ScaleSpec(in, out, s) } +func autoConvert_v1beta1_ScaleStatus_To_extensions_ScaleStatus(in *ScaleStatus, out *extensions.ScaleStatus, s conversion.Scope) error { + out.Replicas = in.Replicas + // WARNING: field 'Selector' requires manual conversion + return nil +} + +func autoConvert_extensions_ScaleStatus_To_v1beta1_ScaleStatus(in *extensions.ScaleStatus, out *ScaleStatus, s conversion.Scope) error { + out.Replicas = in.Replicas + // WARNING: field 'Selector' requires manual conversion + return nil +} + func autoConvert_v1beta1_SupplementalGroupsStrategyOptions_To_extensions_SupplementalGroupsStrategyOptions(in *SupplementalGroupsStrategyOptions, out *extensions.SupplementalGroupsStrategyOptions, s conversion.Scope) error { out.Rule = extensions.SupplementalGroupsStrategyType(in.Rule) if in.Ranges != nil {