-
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
Deprecate v1 projects #1137
Deprecate v1 projects #1137
Conversation
Welcome @hypnoglow! |
Hi @hypnoglow. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
cmd/main.go
Outdated
@@ -188,3 +191,20 @@ func getProjectVersion() (bool, string) { | |||
} | |||
return true, projectInfo.Version | |||
} | |||
|
|||
func printV1DeprecationWarning() { | |||
fmt.Printf(`=============== DEPRECATION WARNING =============== |
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.
If this looks too aggressive, let me know how to rephrase/reformat it.
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 can try making it more concise. How about:
warning: [deprecation] v1 projects are deprecated and will not be supported beyond Feb 1, 2020. Refer to https://book.kubebuilder.io/migration/guide.html for more details.
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.
Thanks for the suggestion! Done
cmd/main.go
Outdated
|
||
func printV1DeprecationWarning() { | ||
fmt.Printf(`=============== DEPRECATION WARNING =============== | ||
v1 projects are deprecated. The support for v1 projects will be dropped at 1 February 2020. |
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 dropped an approximate date, please correct me if it is wrong.
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.
What about a broad date such as "first 2020 quarter"? @droot
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.
Giving a specific date makes the message more clear and that's the intent behind 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.
Thanks for working on this. A few minor suggestions.
cmd/main.go
Outdated
@@ -188,3 +191,20 @@ func getProjectVersion() (bool, string) { | |||
} | |||
return true, projectInfo.Version | |||
} | |||
|
|||
func printV1DeprecationWarning() { | |||
fmt.Printf(`=============== DEPRECATION WARNING =============== |
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 can try making it more concise. How about:
warning: [deprecation] v1 projects are deprecated and will not be supported beyond Feb 1, 2020. Refer to https://book.kubebuilder.io/migration/guide.html for more details.
cmd/main.go
Outdated
|
||
func printV1DeprecationWarning() { | ||
fmt.Printf(`=============== DEPRECATION WARNING =============== | ||
v1 projects are deprecated. The support for v1 projects will be dropped at 1 February 2020. |
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.
Giving a specific date makes the message more clear and that's the intent behind this.
@@ -112,7 +113,9 @@ func main() { | |||
) | |||
|
|||
foundProject, version := getProjectVersion() | |||
if foundProject && version == "1" { | |||
if foundProject && version == project.Version1 { |
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.
Cobra command library does provide mechanism to mark commands as deprecated but I think the implemented approach will work better for our case.
a7d108c
to
1cf7b16
Compare
/ok-to-test |
cmd/main.go
Outdated
@@ -188,3 +191,7 @@ func getProjectVersion() (bool, string) { | |||
} | |||
return true, projectInfo.Version | |||
} | |||
|
|||
func printV1DeprecationWarning() { | |||
fmt.Printf("warning: [deprecation] v1 projects are deprecated and will not be supported beyond Feb 1, 2020. Refer to https://book.kubebuilder.io/migration/guide.html for more details.\n") |
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.
WDYT about:
fmt.Printf("warning: [deprecation] v1 projects are deprecated and will not be supported beyond Feb 1, 2020. Refer to https://book.kubebuilder.io/migration/guide.html for more details.\n") | |
fmt.Printf("warning: [deprecation] v1 projects are deprecated and will not be supported beyond Feb 1, 2020. See the https://book.kubebuilder.io/migration/guide.html for know how to migrate the project to from v1 to v2.\n") |
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.
@camilamacedo86 Is the "See ... for know how to ..." correct English? Sorry for this question, I'm not a native speaker, thus I never heard this idiom :)
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.
@camilamacedo86 ping, I would be thankful if you clarify this moment for me.
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.
... for know how to ...
and project to from
are definetely typos. I would go for ... Refer to https://.. for instructions on how to migrate to v2.\n
. Saying from v1
doesn't give that much info as you already said that v1
is the one being deprecated.
Just my 2 cents.
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.
@Adirio agreed, thanks for help.
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 was not clear in my suggestion. Sorry. It is updated bellow.
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.
Besides the nit . It shows /lgtm for me.
@@ -112,7 +113,9 @@ func main() { | |||
) | |||
|
|||
foundProject, version := getProjectVersion() | |||
if foundProject && version == "1" { | |||
if foundProject && version == project.Version1 { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
Why do you suggest to do this change from another PR (#1143) into 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.
Hi @Adirio,
Because this PR is doing/replacing if foundProject && version == "1" {
for if foundProject && version == project.Version1 {
.
So, as part of its review, I am suggesting if foundProject && version == "1" {
for if hasProjectFile && projectVersion == project.Version1 {
instead of.
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 agree that @camilamacedo86 suggested naming is more suitable. But I also agree that we can lay it on #1143, as the narrow scope of my change was only to replace the string "1"
with the constant project.Version1
, and not to change other things.
If you still think we should introduce this change here, let me know.
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.
@hypnoglow The change she is suggesting is not just a more suitable name, it has to do with the package with the same alias (version
). The context is missing as this is a change I suggested in #1143 and she suggested a name change. Suggesting to change this here makes no sense as this PR is about a completely different topic.
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 is not a big deal so I am OK with.
1cf7b16
to
fd98bde
Compare
/assign @camilamacedo86 |
cmd/main.go
Outdated
@@ -188,3 +191,7 @@ func getProjectVersion() (bool, string) { | |||
} | |||
return true, projectInfo.Version | |||
} | |||
|
|||
func printV1DeprecationWarning() { | |||
fmt.Printf("warning: [deprecation] v1 projects are deprecated and will not be supported beyond Feb 1, 2020. Refer to https://book.kubebuilder.io/migration/guide.html for instructions on how to migrate to v2.\n") |
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.
My suggestion is to keep the text more friendly as it is done in its docs + few nits.
// add const on top
const (
NoticeColor = "\033[1;36m%s\033[0m"
)
fmt.Printf("warning: [deprecation] v1 projects are deprecated and will not be supported beyond Feb 1, 2020. Refer to https://book.kubebuilder.io/migration/guide.html for instructions on how to migrate to v2.\n") | |
fmt.Printf(NoticeColor,"[Deprecation Notice] : v1 projects are deprecated and will not be supported beyond Feb 1, 2020.\n See how to upgrade your project to v2: https://book.kubebuilder.io/migration/guide.html\n") |
Following its result.
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.
Done
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 tested it working locally. It is working for the init command and others as requested in the issue. 🥇 Well done. Just apply the suggested nit and I think it will be ok to be merged.
fd98bde
to
9ceebf0
Compare
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.
/assign @diroot |
@camilamacedo86: GitHub didn't allow me to assign the following users: diroot. Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. In response to this:
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.
/lgtm
/approve
Thanks for working on this.
Let's get this in!
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hypnoglow, mengqiy 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 |
This PR deprecates v1 projects. Fixes #1136