-
Notifications
You must be signed in to change notification settings - Fork 1.4k
✨clusterctl: add upgrade plan cmd #2178
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
✨clusterctl: add upgrade plan cmd #2178
Conversation
49eea4a
to
eb4de58
Compare
eb4de58
to
727a0fd
Compare
url: "https://github.com/kubernetes-sigs/cluster-api-provider-aws/releases/latest/infrastructure-components.yaml", | ||
providerType: clusterctlv1.InfrastructureProviderType, | ||
}, | ||
&provider{ | ||
name: "docker", | ||
name: DockerProviderName, |
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 aren't maintaining github.com/kubernetes-sigs/cluster-api-provider-docker as a separate repo any more. See #1565 for example. We may want to consider removing capd from clusterctl for now, and add it back later when we have updated release artifacts for 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.
What about assuming infrastructure-components.yaml for CAPD will get released with CAPI (might be with another file 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.
That's fair, but we haven't decided what it will look like yet.
return nil, err | ||
} | ||
|
||
// UpgradePlan is an alias for cluster.UpgradePlan; this makes the conversion |
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.
icky but ok i guess
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'm not super happy about this, but I don't have a better idea...
return err | ||
} | ||
|
||
//TODO: switch to klog as soon as https://github.com/kubernetes-sigs/cluster-api/pull/2150 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.
Seems we can switch now?
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.
Unfortunately no, because we switched to the global logger approach and this is not yet merged. Waiting for #2191
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.
Since 2191 is blocked on this PR, we can create an issue to track this work or I'm assuming @fabriziopandini will be submitting a PR to refactor the upgrade cmd to use the global logger.
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.
He's going to update 2191 after this goes in
b0a7ba9
to
1b99813
Compare
@fabriziopandini: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. 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 understand the commands that are listed here. |
1b99813
to
936d92b
Compare
936d92b
to
43e6ca6
Compare
rebased |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini, ncdc 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 |
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
What this PR does / why we need it:
This PR adds the
clusterctl upgrade plan
cmd as per discussion on #1988Which issue(s) this PR fixes:
rif #1729
/area clusterctl
/assign @ncdc
/assign @vincepri