Skip to content
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.

Sign up for GitHub

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

Jump to bottom

✨clusterctl: read templates from different sources #2265

Merged
merged 1 commit into from
Feb 11, 2020
Merged

✨clusterctl: read templates from different sources #2265

merged 1 commit into from
Feb 11, 2020

Conversation

fabriziopandini
Copy link
Member

What this PR does / why we need it:
This PR introduces to clusterctl support for reading cluster templates from a location different than the provider repository.

Supported locations are GitHub repository, local file system, and ConfiMaps.

It also adds support for returning the list of variables in a template.

Which issue(s) this PR fixes:
Fixes #2133

/area clusterctl
/assign @ncdc
/assign @vincepri

@k8s-ci-robot k8s-ci-robot added area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 5, 2020
@vincepri
Copy link
Member

vincepri commented Feb 6, 2020

/assign

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2020
This was referenced Feb 6, 2020
Merged
Closed
@ncdc ncdc added this to the v0.3.0 milestone Feb 7, 2020
@wfernandes
Copy link
Contributor

/assign

wfernandes
wfernandes reviewed Feb 7, 2020
View reviewed changes
cmd/clusterctl/pkg/client/cluster/template.go Outdated
}

if fileContent.Encoding == nil || *fileContent.Encoding != "base64" {
return nil, errors.Errorf("invalid encoding detected for %q. Only base 64 encoding supported", rURL.Path)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

Suggested change
return nil, errors.Errorf("invalid encoding detected for %q. Only base 64 encoding supported", rURL.Path)
return nil, errors.Errorf("invalid encoding detected for %q. Only base64 encoding supported", rURL.Path)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also wanted to point out that we are just printing the rURL.Path and not rURL.String(). Not sure if that was the behavior you wanted. 🙂

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found printing rURL.Path less verbose, but I don't have strong opinions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool...I'm fine with it. Just wanted to confirm expected behavior.

vincepri
vincepri reviewed Feb 10, 2020
View reviewed changes
cmd/clusterctl/cmd/config_cluster.go Outdated
@@ -33,6 +34,13 @@ type configClusterOptions struct {
kubernetesVersion string
controlPlaneMachineCount int
workerMachineCount int

URL string
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this meant to be exported?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also do you want to use net.URL to have some sort of validation?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are validating inside the library

cmd/clusterctl/cmd/config_cluster.go Show resolved Hide resolved
cmd/clusterctl/pkg/client/cluster/template.go Show resolved Hide resolved
cmd/clusterctl/pkg/client/cluster/template.go Outdated Show resolved Hide resolved
cmd/clusterctl/pkg/client/cluster/template.go Show resolved Hide resolved
cmd/clusterctl/pkg/client/cluster/template.go Outdated Show resolved Hide resolved
cmd/clusterctl/pkg/client/cluster/template.go Outdated Show resolved Hide resolved
cmd/clusterctl/pkg/client/config.go
@@ -47,19 +48,150 @@ func (c *clusterctlClient) GetProviderComponents(provider, targetNameSpace, watc
return components, nil
}

// GetClusterTemplateOptions carries the options supported by GetClusterTemplate.
type GetClusterTemplateOptions struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
type GetClusterTemplateOptions struct {
type GetClusterTemplateInput struct {

Consider this for more clarrity to users

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no strong opinion, but all the other types in input to high-level types use options: InitOptions, DeleteOptions, MoveOptions, PlanUpgradeOptions, so I would prefer to keep this consistent (leave as it is or change all, but in another PR)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Options tells me I'm configuring something, but here and in the other places you mentioned, the structs are really just used as input to avoid a bunch of arguments. We can refactor later, but to keep it consistent with other parts of Cluster API and providers, Input/Output is preferred

cmd/clusterctl/pkg/client/config.go
}

// numSources return the number of template sources currently set on a GetClusterTemplateOptions.
func (o *GetClusterTemplateOptions) numSources() int {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method seems to be used to check if the source is valid, seems that having more than one source isn't something we want to support. For that reason I'd suggest renaming to validateSources() error

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to know the number of sources

  • if > 1 error
  • if == 0 use Respostiotry as a default source (values will be inferred from the cluster inventory)

@fabriziopandini
Copy link
Member Author

@vincepri comment addressed!

vincepri
vincepri approved these changes Feb 10, 2020
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@fabriziopandini squash and I think we should be good to go
@wfernandes over to you for lgtm

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, vincepri

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 10, 2020
@wfernandes
Copy link
Contributor

@fabriziopandini Other than the %q mentioned in this comment https://github.com/kubernetes-sigs/cluster-api/pull/2265/files#r377223703 and squash, it lgtm.

@fabriziopandini
Copy link
Member Author

@wfernandes comment addressed + squashed
Thanks for feedback!

vincepri
vincepri approved these changes Feb 11, 2020
View reviewed changes
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@wfernandes

@wfernandes
Copy link
Contributor

@fabriziopandini Looks like there are some conflicting files.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2020
@fabriziopandini
Copy link
Member Author

@wfernandes rebased

@wfernandes
Copy link
Contributor

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 11, 2020
@k8s-ci-robot k8s-ci-robot merged commit e939e07 into kubernetes-sigs:master Feb 11, 2020
@fabriziopandini fabriziopandini deleted the clusterctl-config-from branch February 11, 2020 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wfernandes wfernandes wfernandes left review comments

@vincepri vincepri vincepri approved these changes

@justinsb justinsb Awaiting requested review from justinsb

Assignees

@ncdc ncdc

@wfernandes wfernandes

@vincepri vincepri

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/clusterctl Issues or PRs related to clusterctl cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.

Allow clusterctl to specify a custom Workload Cluster template
5 participants
@fabriziopandini @vincepri @wfernandes @k8s-ci-robot @ncdc