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: move show-join-command as a separate phase #111512
Conversation
@SataQiu: 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. |
a7b1e24
to
1647f78
Compare
@@ -96,11 +96,12 @@ func NewBootstrapTokenStringFromIDAndSecret(id, secret string) (*BootstrapTokenS | |||
|
|||
// BootstrapTokenToSecret converts the given BootstrapToken object to its Secret representation that | |||
// may be submitted to the API Server in order to be stored. | |||
func BootstrapTokenToSecret(bt *BootstrapToken) *v1.Secret { | |||
func BootstrapTokenToSecret(bt *BootstrapToken, labels map[string]string) *v1.Secret { |
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.
hm, i know this function is just for kubeadm use mainly but with this change we are modifying the signature. its a breaking change in a v1 package.
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 could introduce a new function called BootstrapTokenToSecretWithLabels
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.
Based on our previous discussion, maybe we shouldn't support separate calls kubeadm init phase show-join-command
. Because it has a strong dependence on BootstrapToken
and UploadCerts
phases.
Our current workflow framework does not support dependency checking, and perhaps we need to re-evaluate whether this is necessary... @neolit123
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 we are adding the phase it makes sense to support calling it as a CLI, but i think we should just error out because the dependencies are not there. i guess the main purpose would be for users to be able to skip 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.
In order to implement dependency verification, I introduce a new field Dependencies
for Phase
.
I think this is fine, because it is not a break change, the original behavior will not be affected.
What do you think? @neolit123
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 you run kubeadm init phase show-join-command
, the error output will be something like this:
unresolved dependencies:
missing [bootstrap-token upload-certs] phase(s) needed by "show-join-command" phase
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 one problem with the dependencies concept is how do users execute dependencies? it is technically not possible to run two phases one after another to solve the dependencies problem. they can only skip all phases except the target phase and it's dependencies.
but overall i think this is mostly OK.
/hold |
1647f78
to
261e5e8
Compare
} | ||
for _, dep := range p.Phase.Dependencies { | ||
if _, ok := visited[dep]; !ok { | ||
return errors.Errorf("unresolved dependencies: missing %q phase which is needed by %q phase", dep, p.Phase.Name) |
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 could add all the missing dependencies in a new slice and print it with %v, then return a single error
it would look like [a , b]
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.
Updated, the output will be:
unresolved dependencies:
missing [bootstrap-token upload-certs] phase(s) needed by "show-join-command" phase
release note:
suggesting minor change to:
please adjust further if needed. |
dc59381
to
621e610
Compare
621e610
to
8e4cf3b
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
/approve
thanks @SataQiu
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: neolit123, SataQiu 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 |
/hold cancel |
What type of PR is this?
/kind bug
/kind feature
What this PR does / why we need it:
kubeadm: move show-join-command as a separate phase
Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#2734
Special notes for your reviewer:
Does this PR introduce a user-facing change?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: