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.

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 config cluster --list-variables should show required variables #4258

Closed
CecileRobertMichon opened this issue Mar 5, 2021 · 16 comments · Fixed by #4645
Closed
Assignees
Labels
area/clusterctl Issues or PRs related to clusterctl good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Milestone

Comments

@CecileRobertMichon
Copy link
Contributor

CecileRobertMichon commented Mar 5, 2021

User Story

As a user, I would like to know which variables are required and which ones have defaults when I run --list-variables.

Detailed Description

Right now, clusterctl config cluster cluster --list-variables --infrastructure=azure:v0.4.12 returns:

Variables:
  - AZURE_CONTROL_PLANE_MACHINE_TYPE
  - AZURE_LOCATION
  - AZURE_NODE_MACHINE_TYPE
  - AZURE_RESOURCE_GROUP
  - AZURE_SSH_PUBLIC_KEY_B64
  - AZURE_SUBSCRIPTION_ID
  - AZURE_VNET_NAME
  - CLUSTER_NAME
  - CONTROL_PLANE_MACHINE_COUNT
  - KUBERNETES_VERSION
  - WORKER_MACHINE_COUNT

But out of these variables, only 5 of them are actually required.

clusterctl config cluster my-cluster --infrastructure=azure:v0.4.12
Error: value for variables [AZURE_CONTROL_PLANE_MACHINE_TYPE, AZURE_LOCATION, AZURE_NODE_MACHINE_TYPE, AZURE_SUBSCRIPTION_ID, KUBERNETES_VERSION] is not set. Please set the value using os environment variables or the clusterctl config file

I would like the output of list-variables to look something like this instead:

Required Variables:
  - CLUSTER_NAME
  - AZURE_CONTROL_PLANE_MACHINE_TYPE
  - AZURE_NODE_MACHINE_TYPE
  - AZURE_LOCATION
  - AZURE_SUBSCRIPTION_ID
  - KUBERNETES_VERSION

Optional Variables:
  - AZURE_VNET_NAME
  - AZURE_RESOURCE_GROUP
  - AZURE_SSH_PUBLIC_KEY_B64
  - CONTROL_PLANE_MACHINE_COUNT
  - WORKER_MACHINE_COUNT 

Or, even better, give me the default values (but this might be a bit harder to implement):

Required Variables:
  - CLUSTER_NAME
  - AZURE_CONTROL_PLANE_MACHINE_TYPE
  - AZURE_NODE_MACHINE_TYPE
  - AZURE_LOCATION
  - AZURE_SUBSCRIPTION_ID
  - KUBERNETES_VERSION
 
Optional Variables:
  - AZURE_VNET_NAME (defaults to "${CLUSTER_NAME}-vnet")
  - AZURE_RESOURCE_GROUP (defaults to "${CLUSTER_NAME}")
  - AZURE_SSH_PUBLIC_KEY_B64  (defaults to "")
  - CONTROL_PLANE_MACHINE_COUNT (defaults to 1)
  - WORKER_MACHINE_COUNT (defaults to 2)

(more or less, no need to follow this exact formatting).

Note that CLUSTER_NAME is a special case. It's technically required and not defaulted in the template but it does get set in the clusterctl command so it's not required to be set as an environment variable. It might make more sense to omit it altogether or to put it down as a separate category "other variables" that makes it clear it should be set as an arg in the command. Same thing about variables that get defaulted by clusterctl like CONTROL_PLANE_MACHINE_COUNT.

Anything else you would like to add:

[Miscellaneous information that will assist in solving the issue.]

https://github.com/kubernetes-sigs/cluster-api/blob/master/cmd/clusterctl/client/yamlprocessor/simple_processor.go#L65

variables ais a map[string]bool where the value is whether the variable has a default so we should be able to use the value to separate the variables into "required" and "optional" variables.

/kind feature

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Mar 5, 2021
@CecileRobertMichon
Copy link
Contributor Author

/help
/good-first-issue

@k8s-ci-robot
Copy link
Contributor

@CecileRobertMichon:
This request has been marked as suitable for new contributors.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-good-first-issue command.

In response to this:

/help
/good-first-issue

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.

@k8s-ci-robot k8s-ci-robot added good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. labels Mar 5, 2021
@CecileRobertMichon
Copy link
Contributor Author

@fabriziopandini let me know what you think about this

@fabriziopandini
Copy link
Member

fabriziopandini commented Mar 5, 2021

The request makes sense to me
Previously we had #3422 about this topic, but I prefer this one because I like better the proposed output.

FYI there is also #3968 somehow related to this.

@fabriziopandini
Copy link
Member

/area clusterctl

@k8s-ci-robot k8s-ci-robot added the area/clusterctl Issues or PRs related to clusterctl label Mar 5, 2021
@fabriziopandini
Copy link
Member

/milestone v0.4.0

@k8s-ci-robot k8s-ci-robot added this to the v0.4.0 milestone Mar 5, 2021
@CecileRobertMichon
Copy link
Contributor Author

/assign @prankul88

@prankul88 feel free to unassign if you're no longer working on #3422

@fabriziopandini
Copy link
Member

/lifecycle active
/priority important-soon

@k8s-ci-robot k8s-ci-robot added lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Mar 10, 2021
@prankul88
Copy link
Contributor

/unassign

@prankul88
Copy link
Contributor

/assign @mabikash

@k8s-ci-robot
Copy link
Contributor

@prankul88: GitHub didn't allow me to assign the following users: mabikash.

Note that only kubernetes-sigs members, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time.
For more information please see the contributor guide

In response to this:

/assign @mabikash

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.

@sgirem463
Copy link
Contributor

/assign @sgirem463

@sgirem463 sgirem463 removed their assignment Mar 17, 2021
@sgirem463
Copy link
Contributor

@CecileRobertMichon I am a new contributor trying to find an open issue to work on. This issue is unassigned but seems #4262 will fix it. Two questions:

  1. Is it a common practice to open a new PR when fixing an issue (like ⚠️ [clusterctl] Show required and optional values in --list-variables #4262 was opened to fix this Clusterctl config cluster --list-variables should show required variables #4258)
  2. What is a reliable way to find unassigned issues to work on?

Thanks!

@CecileRobertMichon
Copy link
Contributor Author

Hi @sgirem463 and welcome! Usually, you want to comment "/assign" on an issue you are planning on working on so others know that are already working on it. In this case, it was likely an oversight, I think @mabikash is also a new contributor 🙂

@mabikash, can you please comment /assign below so others know you are already working on this?

What is a reliable way to find unassigned issues to work on?

What you did, looking for issues labeled "help-wanted" or "good-first-issues" with no one assigned should usually help you find issues looking for contributors.

See https://github.com/kubernetes-sigs/cluster-api/blob/master/CONTRIBUTING.md for more info on contributing to Cluster API.

@mabikash
Copy link

/assign

@mboersma
Copy link
Contributor

mboersma commented May 10, 2021

@mabikash are you working on this? I have a branch that implements these changes in clusterctl, but I don't want to interfere if you're making progress.

Edit: Oops, didn't see #4262. My PR looks quite similar. :-) Let me know if I can help get it over the finish line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/clusterctl Issues or PRs related to clusterctl good first issue Denotes an issue ready for a new contributor, according to the "help wanted" guidelines. help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. kind/feature Categorizes issue or PR as related to a new feature. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
7 participants