-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
✨ Improve clusterctl upgrade syntax. Don't require namespace #7376
✨ Improve clusterctl upgrade syntax. Don't require namespace #7376
Conversation
5b6f09e
to
c3bc0b3
Compare
cc @ykakarap |
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.
PR looks great, I will document new syntax in the clusterctl upgrade apply help as well in the book in https://cluster-api.sigs.k8s.io/clusterctl/commands/upgrade.html (not sure if there are other places as well)
I will also add a deprecation notice from the previous format in https://main.cluster-api.sigs.k8s.io/developer/providers/v1.2-to-v1.3.html
c3bc0b3
to
3127e1a
Compare
Awesome! Thank you @fabriziopandini for the review and fixing the docs part that I totally forgot about. |
3127e1a
to
71770d8
Compare
71770d8
to
4140303
Compare
I had some downtime so I upgraded the documentation to cover this change. |
4140303
to
e95cfe1
Compare
Since this change will earliest be in 1.4 due to the feature freeze, I will rebase it when there is a separate 1.3 branch and we have an v1.3-to-v1.4 document. But if there is any feedback before them I will of course fix it immediately. |
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.
@oscr thanks for being so patient.
Changes seem ok to, I will only make this more visible to CLI users and to people using clusterctl as a library
e95cfe1
to
6f46d20
Compare
6f46d20
to
03d4a83
Compare
03d4a83
to
974893f
Compare
PR has now been rebased with main, changes added to a |
726cecf
to
5c2cf0c
Compare
5c2cf0c
to
035a5ae
Compare
@ykakarap @fabriziopandini When you have some time, what would be the next steps for this PR? I think we lost a bit track of 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.
Sounds good to me. We can merge independent of the release when we're ready, but I wouldn't force it into v1.3.0. We can discuss if the PR is eligible for cherry-pick after merge. |
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.
Overall LGTM. Just a couple comments.
035a5ae
to
8eb5a2f
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.
/lgtm
/assign @fabriziopandini for approval. |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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 |
@ykakarap @fabriziopandini @sbueringer Thank you once again for all your help and feedback with this pr 🙏 |
What this PR does / why we need it:
Here is the shorted output of a script that creates a quickstart cluster and then performs a
clusterctl upgrade
with the improved syntax.If we try to upgrade docker again with the current (unmodified) clusterctl version:
I also sneaked in some spelling mistake fixes where I found them during development.
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):Fixes #7319