Split internal/current config from v1alpha1 and start using k8s.Scheme #137
Conversation
k8s-ci-robot
added
the
cncf-cla: yes
k8s-ci-robot
added
the
size/XXL
| cfg = &layout.Config | ||
| rawCfg = layout | ||
| } | ||
| err = yaml.UnmarshalStrict(raw, rawCfg) |
There was a problem hiding this comment.
this does unfortunately mean we lose strict unmarshalling (See the config issue for dicussion) but it is currently par for the course in kubernetes tooling.
There was a problem hiding this comment.
strict unmarshal warnings (or errors?) would be pretty easy to add after this PR is merged as a pre-step before Decode.
There was a problem hiding this comment.
had a look at how to create a custom decoder too. seems doable in the near future.
There was a problem hiding this comment.
@BenTheElder, considering above comments it seems to me that this can be acceptable for now right?
There was a problem hiding this comment.
this can be acceptable for now right
yes,
it's just that i recommend that we enable strict unmarshal with the first kind release.
since we cannot merge a strict decoded in api-machinery in this release, we should probably go for the way kubeadm is doing it. calling yaml.UnmarshalStrict before Decode.
There was a problem hiding this comment.
yes, sorry, I wanted to call out that we had this functionality change, but it makes sense.
we can sort out strict unmarshal again later, I'm ok with that not being in the first "release" since the first release will be very alpha grade and not stable. I'm going to make this clear with the versioning of the APIs and kind itself.
we should get strict unmarshal before we make compatibility guarantees between versions so it isn't a breaking change later.
There was a problem hiding this comment.
Awesome to see this done! I added a few comments. Mostly based on having looked at other Kubernetes controller/software. We may not want to adopt all the same patterns - I'm not sure exactly why all these things are the case, but they do seem to be applied consistently 😄
| // Package v1alpha1 implements the v1alpha1 apiVersion of the `kind` Config | ||
| // | ||
| // +k8s:deepcopy-gen=package | ||
| // +k8s:conversion-gen=sigs.k8s.io/kind/pkg/cluster/config/ |
There was a problem hiding this comment.
(not necessarily for this PR): perhaps we should move this to sigs.k8s.io/kind/pkg/apis/kind[/v1alpha1] for consistency with other k8s api packages?
There was a problem hiding this comment.
would sigs.k8s.io/kind/pkg/apis/kind[/v1alpha1] imply conversion-gen-external-types?
e.g. how it's done for the kubelet:
https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/apis/config/v1beta1/doc.go#L18-L19
There was a problem hiding this comment.
@munnerz you are right, this is not necessary now, but this will be used as soon as v1alpha2 will be added, so if you don't mind I will leave it
There was a problem hiding this comment.
+1, let's do this in a follow up.
I put it under pkg/cluster/config in an attempt to signal the scope of the config object, let's consider that as we improve the config / API structure, I'd like to leave room to grow in new and unexpected ways ... other container runtimes, possibly CLI tools ... :-)
fabriziopandini
force-pushed
the
internal-config-version
branch
from
6f8d3d7 to
62dda42
Compare
|
Rebase + squash commits |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: BenTheElder, fabriziopandini The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
…s_cidr_elegible [EOS-11199][GCP][AZURE(VM)] Parametrizar ipPool
This PR sets the ground for addressing #130
Preparation works include the refactor of the current kind config by splitting internal/current from v1alpha1 API versions and switching to k8s.Scheme for managing encoding, conversions, and defaulting.
Note for reviewers