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
kubeadm: Inherit dry-run
flags for each sub-phases
#112945
Conversation
@chendave: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The 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. |
I went through all those command and sub-phases and inherit the flag for all those phases that |
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 think this makes sense. does it enable it for all sub phases that we have? do normal phases already have it?
one request, please start the release note with "kubeadm: ...."
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: chendave, neolit123 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 |
enabled all sub phases where the
yes, releasenote: kubeadm: kubeadm sub-phases now able to support the |
/release-note-edit: kubeadm: kubeadm sub-phases now able to support the dry-run mode, e.g. kubeadm reset phase cleanup-node --dry-run |
just edit it manually in the op like this
|
Done |
If I understand correctly, kubernetes/cmd/kubeadm/app/cmd/init.go Lines 126 to 133 in 575031b
|
mostly true, but I don't want the dry-run to be a flag where it's not applicable, e.g. kubernetes/cmd/kubeadm/app/cmd/phases/join/controlplanejoin.go Lines 85 to 96 in 99360e5
or kubernetes/cmd/kubeadm/app/cmd/phases/init/showjoincommand.go Lines 71 to 78 in 99360e5
show the flag for those phase where it is not consumed is confusing. /hold |
I prefer to make all phase consistent. However, your concern makes sense. I'd like to leave this to other owners to decide. Only +0.5 😄 for the consistent way. |
I checked the code again. /lgtm |
agreed, even if a phase's dryrun is a no-op we can have the flag everywhere. |
+1, current workflow doesn't allow us to set the flag for the sub-phases as a whole without listing all of them in the slice of this is a potential chance to optimize the workflow a little bit, but this is a different thing. I will update this change in my next day. |
- The sub-phases like `kubeadm reset phase cleanup-node` which could be run independently would be able to support the `dry-run` mode as well. - Consistent with the sub-phases which support the `dry-run` mode already, such as `kubeadm init phase control-plane apiserver`. - Prepare for the day when each of those sub-phases could be run independently. Signed-off-by: Dave Chen <dave.chen@arm.com>
some update here,
I think this is good to go now. /hold cancel @pacoxu @neolit123 looking for a |
/lgtm |
The sub-phases like
kubeadm reset phase cleanup-node
which could be run independently would be able to support thedry-run
mode as well.Consistent with the sub-phases which support the
dry-run
mode already, such askubeadm init phase control-plane apiserver --dry-run
.Prepare for the day when each of those sub-phases could be run independently.
Signed-off-by: Dave Chen dave.chen@arm.com
What type of PR is this?
/kind feature
/sig cluster-lifecycle
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: