✨ Refactor clusterctl config to clusterctl generate #4584
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
/cc @fabriziopandini |
| @@ -79,6 +79,7 @@ var configProviderCmd = &cobra.Command{ | |||
| RunE: func(cmd *cobra.Command, args []string) error { | |||
| return runGetComponents() | |||
| }, | |||
| Deprecated: "use `clusterctl generate provider` instead", | |||
There was a problem hiding this comment.
what is the plan for removal of the deprecated commands:
- when would that happen?
- is there an issue to track it?
There was a problem hiding this comment.
The plan is to support the old commands for 0.4 and to remove it in the subsequent release. I've created an issue for tracking it. Do you think we should add a warning about removing it in the Deprecated description string?
k8s-ci-robot
added
the
lgtm
shysank
force-pushed
the
clusterctl_generate_part1
branch
from
8fa9a46 to
4fe2aee
Compare
k8s-ci-robot
removed
the
lgtm
shysank
force-pushed
the
clusterctl_generate_part1
branch
from
4fe2aee to
8a34908
Compare
|
/retest |
k8s-ci-robot
added
the
lgtm
| generateCmd.AddCommand(generateClusterClusterCmd) | ||
| } | ||
|
|
||
| func runGenerateClusterTemplate(cmd *cobra.Command, name string) error { |
There was a problem hiding this comment.
all of this seems to be duplicate code from cmd/clusterctl/cmd/config_cluster.go, can we instead reuse the same helper functions from generate_cluster into config_cluster.go so that the code is shared between the two commands?
There was a problem hiding this comment.
Since we are going anyway to remove config_cluster.go in the next release, I thought it'd be okay to just copy over. Same for the config_provider.go as well.
There was a problem hiding this comment.
If those are duplicates, let's start removing to existing methods in config_cluster.go/config_provider.go and have both the commands to point to the same internal function
There was a problem hiding this comment.
+1 @shysank are you ok with the proposed changes above?
There was a problem hiding this comment.
The functions @CecileRobertMichon is talking about are runGetClusterTemplate and runGenerateClusterTemplate in config_cluster.go and generate_cluster.go respectively. They are almost similar with the difference being the options struct used: configClusterOptions vs generateClusterOptions which are both package level variables (this is the pattern we follow for all the commands). Both the command functions, and options structs are same for now, but will start to diverge in future (eg. with --raw option added). I'm not sure if we want to move them to a common function because it's very specific to the commands. The parts of code which are shareable (eg. printYamlOutput) is already in util.go. That was my rationale for not using the same function for both commands although happy to refactor if people think otherwise, we'll probably have to use the same options struct I think in that case.
There was a problem hiding this comment.
Could we add a comment to that effect on the configClusterOptions?
There was a problem hiding this comment.
One clarification on the above comment. The --raw is for generate_provider. But the idea is still the same given that we are planning to remove config_cluster in 0.5.x, and there may be new options added to generate_cluster.
There was a problem hiding this comment.
given the context above (thanks for the explanation!), I'm fine with current implementation
There was a problem hiding this comment.
Same here, thanks for clarifying. We'll have to be diligent in reviews to make sure no changes are made to the config functions once this merges. It might be good to add a comment as well as Vince suggested.
There was a problem hiding this comment.
Yeah, I'll add comments. Thanks for the feedback!
shysank
force-pushed
the
clusterctl_generate_part1
branch
from
8a34908 to
b535ff1
Compare
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fabriziopandini 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 |
k8s-ci-robot
added
the
approved
|
/retest |
What this PR does / why we need it:
This pr refactors
clusterct config clusterandcluster config providertoclusterctl generate clusterandclusterctl generate providerrespectively. Some changes to be noted:clusterctl generate provider-> generates a yaml template of the provider, this is equivalent toclusterctl config provider -o yamlclusterctl generate provider --describe-> displays information about the provider in text, this is equivalent toclusterctl config providerWhich issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #3360
** There was an earlier PR that aimed to fix the same issue. Based on the discussion, we're splitting it into two prs, this is the first one.