-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
*: remove project version 1 references, update migration guide to 2 -> 3 #1636
Conversation
Skipping CI for Draft Pull Request. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: estroz 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 |
Closes #1508 |
@@ -45,12 +45,10 @@ | |||
|
|||
- [Migrations](./migrations.md) | |||
|
|||
- [Kubebuilder v1 vs v2](./migration/v1vsv2.md) | |||
- [Kubebuilder v2 vs v3](./migration/v2vsv3.md) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @estroz,
IMO we should not remove the V1 to the V2 migration guide. It should still
What do you need is add a new migration V2 to V3 instead.
go test -race ./cmd/... | ||
go test -race ./pkg/... | ||
|
||
fi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where it is used?
Is not a test to check the project in the release process?
So, would not be better it be updated to use the project version 3-alpha instead?
@@ -1,220 +1,58 @@ | |||
# Migration from v1 to v2 | |||
# Migration from v2 to v3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto above
@@ -1,85 +0,0 @@ | |||
# Single Group to Multi-Group |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be removed.
For V2 and V# it still valid.
@@ -1,24 +0,0 @@ | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a PR to remove the V1 mock and update the migration guide to not point to it.
Could we move forward with it #1499 before?
The default `CustomResourceDefinition` version is now `v1`, bumped from `v1beta1` which is | ||
[deprecated][crds-deprecated-doc]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was it merged?
The default `ValidatingWebhookConfiguration` and `MutatingWebhookConfiguration` versions are now `v1`, | ||
bumped from `v1beta1` which is [deprecated][webhook-deprecated-pr]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same ?
@@ -1,56 +0,0 @@ | |||
# Kubebuilder v0 v.s. v1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not remove the old docs. they should still.
@@ -1,146 +0,0 @@ | |||
# Migration guide from v0 project to v1 project |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should not remove the old docs. they should still.
@@ -64,7 +64,7 @@ func readFrom(fs afero.Fs, path string) (c config.Config, err error) { | |||
|
|||
// kubebuilder v1 omitted version, so default to v1 | |||
if c.Version == "" { | |||
c.Version = config.Version1 | |||
return config.Config{}, fmt.Errorf("project version key `version` is empty or does not exist in %s", path) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO we need a PR to ADD the migration guide v2 to v3
and another one to do these changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please, see the comments:
- we should not remove the old docs
- we should not remove the migration guide v1 to v3
- we should just add a new migration guide v2 to v3
- IMO the changes in the code needs be done in one Pr and we need to have another to add the new migration guide.
PS.: I think we ought to merge #1499 before this one.
/hold
/hold |
You're probably right.
Sure |
@estroz: PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments about catching deprecated plugin versions and ensuring we continue to provide helpful messaging about its deprecation.
// The --plugins flag can only be called to init projects v2+. | ||
if c.projectVersion != config.Version1 { | ||
// The --plugins flag can only be called to init projects v3+. | ||
if c.projectVersion != config.Version2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this conditional continue to check c.projectVersion != config.Version1
? Or in the case v1
is used, explicitly print out a message saying that version is now deprecated? We could use the IsVersionSupported()
helper to accomplish this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
V1 is no longer supported and its code no longer exists in the project for a while so I am not sure if it would be required. However, I am ok with the suggestion as well.
@@ -37,11 +37,6 @@ var _ = Describe("Config", func() { | |||
expectedConfig Config | |||
) | |||
|
|||
By("Using config version 1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we keep this check to ensure that v1 usage continues to produce the expected behavior (failure?)
Hi @estroz, The migration guide from V2 to V3 is merged. See: #1714 in this way, I think we can close this. WDYT? |
Hi @estroz, I am closing this one based on the comment #1636 (comment). However, please feel free to re-open if you see that it is required. |
Project version 1 shouldn't be mentioned in code, and any project version that isn't supported by the binary used in a project should cause an error to be returned.
This PR also adds a skeleton migration guide with some TODO's for future work that will likely be in before the next major release.
/kind documentation