Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add go/v3-alpha #4232

Closed

Conversation

camilamacedo86
Copy link
Contributor

@camilamacedo86 camilamacedo86 commented Nov 13, 2020

Description of the change:

  • Add go/v3-alpha
  • Add the same sample generated with go/v3-alpha

Motivation for the change:

  • Support k8s 1.20
  • Allows test the projects built with go/v3-alpha and check the diff between v2 X v3-alpha

Follow-ups

.gitignore Outdated Show resolved Hide resolved
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm still hesitant to add v3-alpha samples because it is in alpha, but I guess we do need to test our integrations. We also need e2e tests for v3-alpha.

Makefile Outdated Show resolved Hide resolved
hack/generate/samples/internal/go/generate_v2.go Outdated Show resolved Hide resolved
"--version", mh.ctx.Version,
"--kind", mh.ctx.Kind,
"--defaulting",
"--defaulting")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"--defaulting")
"--programmatic-validation")

Copy link
Contributor Author

@camilamacedo86 camilamacedo86 Nov 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the sample works with defaulting . If we change it for v3 we need to change it for v2. Otherwise, we are unable to compare both to check what changed besides increasing the changes/mock/test that cannot be re-used both at this moment. But we can do it in a follow-up. See: #4234

PS.: I also think that would be better get it done after we are using the samples in the e2e tests #4056 becuase we can ensure better this change.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've specified --defaulting twice

@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Nov 14, 2020

HI @estroz,

I'm still hesitant to add v3-alpha samples because it is in alpha, but I guess we do need to test our integrations. We also need e2e tests for v3-alpha.

IMO:

Regards samples/tesdata

I agree 100% with you that we make clear to users that it is an alpha plugin. See the changelog.

However, we are providing a new option for the users then, we also need to have an example over that. Otherwise, it makes it very harder and increases our effort either to contribute with the further changes and solve the issues raised in the repo. Note as well that:

  • By running the samples we are ensuring at least the mainly commands/scaffolds provided by the tool with the new plugin
  • The testdata are important and relevant not only to users. We need the samples to be able to compare v2 X v3 and check the diffs, for we are able to very the further changes in the plugins and scaffolds, have a mock sample to test the projects scaffolds with v3-alpha, help use to built the ansible/helm v2 alpha plugins, check/review and validate the next bumps that affect/change ve3-alpha and etc.

Regards e2e tests with v3-alpha

I agree with you. But I'd like to introduce it in a follow after we get merged the #4056 to try to reduce the effort in the CI. WDYT? Could we agree with that?

@joelanford
Copy link
Member

joelanford commented Nov 16, 2020

Total drive-by: Before we merge this, we need to decide what our support statement will be for v3-alpha projects, and if its anything other than "full support like all other plugins AND full backwards-compatibility guarantees", then we need to very clearly document what the support will be and what guarantees and non-guarantees there are for backward and forward compatibility.

I'm worried about telling people they need to use v3-alpha to get access to Kubernetes 1.20 features, and then in the same breath telling them that we can't support them because they're using an alpha feature and that we'll also break their project in the future (e.g. if/when the phase 2 plugins work changes the format of the PROJECT file)

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 16, 2020
@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Nov 16, 2020

I'm worried about telling people they need to use v3-alpha to get access to Kubernetes 1.20 features, and then in the same breath telling them that we can't support them because they're using an alpha feature and that we'll also break their project in the future (e.g. if/when the phase 2 plugins work changes the format of the PROJECT file)

IMO we can:

  • Tell that if users would like to use the 1.20 features than, they can try out the alpha plugin which is available but it is an alpha implementation which means that can sofer breaking changes and etc and that it will ONLY fully supported when this plugin version be stable and the be scaffolded by default.

@joelanford
Copy link
Member

Based on last week's discussion, I think we all agreed that kubebuilder's project version 3 and its go/v3 plugin need to be finalized before we will accept them and begin using them in the SDK.

These milestones are prerequisites for scaffolding 1.20-compatible projects because the version of controller-runtime that will support 1.20 (likely v0.8.0) will inherit the breaking changes made in controller-runtime v0.7.0 that are incompatible with the currently supported go.kubebuilder.io/v2 plugin.

@camilamacedo86
Copy link
Contributor Author

camilamacedo86 commented Nov 26, 2020

Hi @joelanford,

Yes. I will close this one and then, this PR can be used as a helper for when we are able to do it since here we are generating the sample with v3/go plugin.

Pinging @estroz @jmrodri and @varshaprasad96 @asmacdo only for let they know that it has what is required for we add the support to the new plugin as it is being stabilized.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants