adopt Kubernetes's API tooling to automatically generate #36
Conversation
|
Welcome @Lion-Wei! It looks like this is your first PR to kubernetes-sigs/kind 🎉 |
|
/retest |
4dec070 to
89bdfed
Compare
|
Hi, @BenTheElder , PTAL. And I have something not sure:
Not very familiar with those tools, if there are anything misconduct or mistake please let me know. : ) |
|
Thanks for working on this!
I think we may need some variant on Any to still mange the serialization
Yes! That sounds great to me. Imho we should have a v1alpha1 api package, and we should always maintain a |
|
Maybe we should break up the PRs, the first one can handle deepcopy with commits something like like:
Then we can follow up with adopting the other portions of the API tooling? I think it will probably be easier to sort out the interfaces etc if we break it up. |
|
actually re-reading this, its already pretty easy to review :-) |
|
I'll work on a PR to make it possible to ignore golint for the generated API code |
|
Yeah, I was pretty concerned about the commit organize during this work. 😌 |
|
I'm super happy with this PR as a starting point, we just need to sort out the golint failure. I got pulled off looking into #43 :/ |
|
Wow, you are working in midnight...Please take your time... 😅 |
|
Should we keep Any
Based on how Kubernetes projects work, 'Any' doesn't really make sense long
term IMO.
The whole point of conversions is to allow users to write their manifests
in any API version, and the internal machinery will then convert into a
single API version so that 'kind' itself can work with those types without
having to maintain different code paths for different versions.
Wrt serialisation, when it comes to writing the manifest back out to
somewhere, we will write it in whichever version is appropriate (usually
named the 'preferred' version, however there is nothing requiring a
particular version).
It's worth noting as well, that a conversion must preserve *all* fields
when going A->B->A. This means that if a new field is added, or a type
changed, our conversion code *must* preserve this new data on the old type
*somehow* (usually achieved through annotations, adding the new field to
the old version, or some kind of casting/type conversion depending on the
change).
Once done properly, we should be able to encode, decode, convert and
default all via a runtime.Scheme instance, thus removing the need for an
interface like Any.
I'll not be able to take a look at this PR until Monday - but if this
blocks other things then I can always do a retroactive review after merge 😄
Thanks for picking this up!! I am especially looking forward to no longer
writing manually created deep copy functions 😅
…On Thu, 27 Sep 2018 at 08:13, Lion-Wei ***@***.***> wrote:
Wow, you are working in midnight...Please take your time... 😅
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMbP75sBiEr-gkkLZdO9ZArpvjVOx4Mks5ufHqSgaJpZM4W6Bmo>
.
|
|
Deferring to @munnerz on this who has vastly more experience with this tooling and building APIs like this :-) |
|
the gopkg.lock / gopkg.toml conflict now. I'm working on the verify updates |
|
filed #48 which should unblock this once the deps update is rebased / fixed up |
|
oops, filing a fix for the 1-11 job
…On Sat, Oct 6, 2018 at 8:30 PM k8s-ci-robot ***@***.***> wrote:
@Lion-Wei <https://github.com/Lion-Wei>: The following test *failed*, say
/retest to rerun them all:
Test name Commit Details Rerun command
pull-kind-conformance-parallel-1-11 3a9831f
<3a9831f>
link
<https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_kind/36/pull-kind-conformance-parallel-1-11/1/> /test
pull-kind-conformance-parallel-1-11
Full PR test history
<https://k8s-gubernator.appspot.com/pr/kubernetes-sigs_kind/36>. Your PR
dashboard <https://k8s-gubernator.appspot.com/pr/Lion-Wei>. Please help
us cut down on flakes by linking to
<https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests>
an open issue
<https://github.com/kubernetes-sigs/kind/issues?q=is:issue+is:open> when
you hit one in your PR.
Instructions for interacting with me using PR comments are available here
<https://git.k8s.io/community/contributors/guide/pull-requests.md>. If
you have questions or suggestions related to my behavior, please file an
issue against the kubernetes/test-infra
<https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:>
repository. I understand the commands that are listed here
<https://go.k8s.io/bot-commands>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#36 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AA4BqwvtQKyu7-XDPFZEw7NqRa1nsomoks5uiXVygaJpZM4W6Bmo>
.
|
|
/retest
…On Sat, Oct 6, 2018, 20:34 Benjamin Elder ***@***.***> wrote:
oops, filing a fix for the 1-11 job
On Sat, Oct 6, 2018 at 8:30 PM k8s-ci-robot ***@***.***>
wrote:
> @Lion-Wei <https://github.com/Lion-Wei>: The following test *failed*,
> say /retest to rerun them all:
> Test name Commit Details Rerun command
> pull-kind-conformance-parallel-1-11 3a9831f
> <3a9831f>
> link
> <https://k8s-gubernator.appspot.com/build/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_kind/36/pull-kind-conformance-parallel-1-11/1/> /test
> pull-kind-conformance-parallel-1-11
>
> Full PR test history
> <https://k8s-gubernator.appspot.com/pr/kubernetes-sigs_kind/36>. Your PR
> dashboard <https://k8s-gubernator.appspot.com/pr/Lion-Wei>. Please help
> us cut down on flakes by linking to
> <https://git.k8s.io/community/contributors/devel/flaky-tests.md#filing-issues-for-flaky-tests>
> an open issue
> <https://github.com/kubernetes-sigs/kind/issues?q=is:issue+is:open> when
> you hit one in your PR.
>
> Instructions for interacting with me using PR comments are available here
> <https://git.k8s.io/community/contributors/guide/pull-requests.md>. If
> you have questions or suggestions related to my behavior, please file an
> issue against the kubernetes/test-infra
> <https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:>
> repository. I understand the commands that are listed here
> <https://go.k8s.io/bot-commands>.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#36 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AA4BqwvtQKyu7-XDPFZEw7NqRa1nsomoks5uiXVygaJpZM4W6Bmo>
> .
>
|
There was a problem hiding this comment.
This looks good to me, barring a few comments.
I think we will next need to look at:
- Re-arranging packages to follow standard k8s conventions
- Moving encoding/decoding over to use the constructed Scheme, that has these new types registered
- Defining an 'internal' API version, to be used as a base for conversion
- Ensuring the version conversion works (after switching to the new scheme)
We can then begin to consider actually implementing API changes as new versions, as a test 😄
| @@ -42,7 +47,11 @@ required = [ | |||
| name = "github.com/spf13/cobra" | |||
| version = "0.0.3" | |||
|
|
|||
|
|
|||
| [[constraint]] | |||
| branch = "master" | |||
| name = "k8s.io/apimachinery" | |||
There was a problem hiding this comment.
We should be using matching versions of code-generator and apimachinery... perhaps best to switch code-generator to master for now too, if we require master of apimachinery? /cc @BenTheElder
| @@ -16,4 +16,8 @@ limitations under the License. | |||
|
|
|||
| // Package config implements the current apiVersion of the `kind` Config | |||
| // along with some common abstractions | |||
|
|
|||
| // +k8s:deepcopy-gen=package | |||
| // +k8s:conversion-gen=sigs.k8s.io/kind/pkg/cluster/config | |||
There was a problem hiding this comment.
afaict, we don't use conversion-gen right now, so this can be removed?
| @@ -92,7 +92,7 @@ func Unmarshal(raw []byte) (config.Any, error) { | |||
| // load version | |||
| var cfg config.Any | |||
| switch version { | |||
| case config.APIVersion: | |||
| case config.SchemeGroupVersion.String(): | |||
There was a problem hiding this comment.
👍 so for the time being, we use the current conversion interface/logic. SGTM.
|
|
||
| var ( | ||
| // SchemeGroupVersion is group version used to register these objects | ||
| SchemeGroupVersion = schema.GroupVersion{Group: GroupName, Version: "v1alpha1"} |
There was a problem hiding this comment.
Given we are using the kube-generators, I think it makes sense to move to a kube-style package layout too, i.e. pkg/apis/kind/v1alpha1.
Probably best to do that as a separate PR for now though, to reduce the blast radius for this PR! 😄
|
discussed with munnerz, let's get this in and follow up with improving our full migration to the tooling next. I'll send some more sweeping changes today to fix up kind ref #59 |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Lion-Wei, munnerz 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 |
…0927_Create-kind-without-apply-manifest [EOS-10927] Feature/eos 10927 create kind without apply manifest
xref: #21