-
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
Plumb addon support into clusterctl. #376
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: roberthbailey 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 |
Is there any harm in making this an optional flag? Until there's machinery to generate and validate the addons are installed/healthy, this is more of a convenience flag. |
I could make it optional, but I think that setting up the standard storage class (in particular) is going to be required for user supplied addons (e.g. logging & monitoring). I'm happy removing all of the ingress stuff out of the template for GCE and just leaving the storage class (for now) if you think that would be better. |
And apparently I need to update some tests :) |
I can make the flag optional if you have strong opinions. |
Let's keep the GCE one intact. We've had discussions about 'core addons' and on GCE ingress is one of them. On vSphere it isn't because there is no 'standard' ingress. No strong opinions on optionality. We can always make it optional later if it makes sense, let's go with this! |
clusterctl/cmd/create_cluster.go
Outdated
@@ -78,30 +79,35 @@ func RunCreate(co *CreateOptions) error { | |||
if err != nil { | |||
return err | |||
} | |||
ac, err := ioutil.ReadFile(co.AddonComponents) | |||
if err != nil { | |||
return err |
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.
This error message will be cryptic to a user who doesn't know the codebase, please wrap with something like,
fmt.Errorf("error loading addons file '%v': %v", co.AddonComponents, err)
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.
Good catch. I'd just copied the code for provider components from above, so I wrapped that in a nicer error string too.
clusterctl/cmd/create_cluster.go
Outdated
// TODO: Remove as soon as code allows https://github.com/kubernetes-sigs/cluster-api/issues/157 | ||
createClusterCmd.Flags().StringVarP(&co.Provider, "provider", "", "", "Which provider deployment logic to use (google/vsphere)") | ||
|
||
// Optional flags | ||
createClusterCmd.Flags().BoolVarP(&co.CleanupExternalCluster, "cleanup-external-cluster", "", true, "Whether to cleanup the external cluster after bootstrap") | ||
createClusterCmd.Flags().StringVarP(&co.VmDriver, "vm-driver", "", "", "Which vm driver to use for minikube") | ||
createClusterCmd.Flags().StringVarP(&co.KubeconfigOutput, "kubeconfig-out", "", "kubeconfig", "where to output the kubeconfig for the provisioned cluster.") | ||
createClusterCmd.Flags().StringVarP(&co.KubeconfigOutput, "kubeconfig-out", "", "kubeconfig", "where to output the kubeconfig for the provisioned cluster") |
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.
Good that we made this consistent -- I'm of the inclination that we should have a period at the end of each of these descriptions so that the inevitable description that is more than one sentence won't look out of place. That can come later though.
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 just removed this one since it was out of place. Making them consistent with periods is fine with me though if you think that will cause less churn later. I'll fix it now since I have to regenerate all of the *.golden files as it is...
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 don't really like the way that they are output with periods at the end (since the default value is appended by the library code after our text) so I'm not going to do it now. We can debate that in a separate PR. :)
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.
Makes sense, wasn't aware of that.
@spew PTAL |
/lgtm |
docs/getting_started.md: use new out directory for generate-yaml.sh
What this PR does / why we need it: This PR adds yet one more required flag to clusterctl to specify "addons". For the purposes of this PR, "addons" are resources that are applied to the internal cluster (e.g. gce) but not the external cluster (minikube). So you can put things that are specific to the environment into addons.yaml (like storageclasses) which would fail if you tried to apply them to minikube.
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 #
Special notes for your reviewer:
Release note:
@kubernetes/kube-deploy-reviewers