-
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
📖 clusterctl: initial set of documentation #1994
📖 clusterctl: initial set of documentation #1994
Conversation
8dac15d
to
78e9694
Compare
@vincepri @vincepri |
@ncdc many thanks for the feedback, really appreciated! |
docs/book/src/clusterctl/contract.md
Outdated
|
||
The `clusterctl config cluster` command allows user to set a small set of common variables via CLI flags or command arguments. | ||
|
||
Templates writers should use the common variables to ensure consistency across providers and a simpler user experience |
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.
(less of a comment of the documentation itself but possibly something address in the future)
The absence of some these variables can lead to surprises in the UX. Say, if a template is opinionated about the number of control plane nodes it should create (e.g. a 'dev' template that hard codes that number to 1 to minimize resource usage), a user attempting to use said template while specifying
... --controlplane-machine-count 3 --worker-machine-count 5
would see only part of his intent satisfied in an otherwise fully functional configuration without warning
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 is correct.
The authors of templates should provide documentation, and in case the template is opinionated, also the underlying assumptions (e.g. 1 worker to minimize resources) should be documented.
However, if this became a problem, we should always change this into a minimal set of required variables, but TBH, I fear this is too much restrictive ATM
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.
First pass, thanks for putting this together!
Regarding the structure I took inspiration / copied what kubectl has todayhttps://kubernetes.io/docs/reference/kubectl/overview which most users will be already familiar with.
docs/book/src/SUMMARY.md
Outdated
- [init](./clusterctl/init.md) | ||
- [move](./clusterctl/move.md) | ||
- [upgrade](./clusterctl/upgrade.md) | ||
- [config cluster](clusterctl/config-cluster.md) | ||
- [adopt](clusterctl/adopt.md) | ||
- [delete](clusterctl/delete.md) |
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.
Let's put these underclusterctl Commands
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 assume this should be a new subtree in the summary (done)
docs/book/src/clusterctl/intro.md
Outdated
|
||
See the [kind documentation](https://minikube.sigs.k8s.io/) for more details. | ||
{{#/tab }} | ||
{{#tab kind for docker provider}} |
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'd honestly keep everything about CAPD separately, although I want to hear from other folks first
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.
Just a couple minor things. LGTM!
Doing another pass |
docs/book/src/SUMMARY.md
Outdated
- [init](./clusterctl/init.md) | ||
- [config cluster](clusterctl/config-cluster.md) | ||
- [move](./clusterctl/move.md) | ||
- [adopt](clusterctl/adopt.md) | ||
- [upgrade](./clusterctl/upgrade.md) | ||
- [delete](clusterctl/delete.md) |
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.
Prefix these files with command-
? Such as ./clusterctl/command-init.md
?
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 moved all the command files into a command folder, is it ok for you?
docs/book/src/clusterctl/contract.md
Outdated
The `clusterctl` command can ship with embedded metadata for pre-defined providers. | ||
If, as a provider implementer, you are interested to this feature, please send a PR to the Cluster API repository. |
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 seems the same as line 24-25
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.
24-25 talks about the embedded list of providers
here it is talking about the embedded metadata
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.
/approve
/assign @ncdc
for final lgtm
[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 |
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.
These are my last review comments 😄. Please fix and then let's merge. We can open new PRs to fix more if needed.
506026c
to
eea1679
Compare
@ncdc comments addressed + squashed commits |
/lgtm |
What this PR does / why we need it:
This PR creates the first set of documents for clusterctl:
Being clusterctl a work in progress, also those pages should be considered WIP. At the same time, it is important to start sharing current assumptions and collect feedbacks
Which issue(s) this PR fixes
Fixes: #1490
Rif #1729
/assign @vincepri
/cc @akutz @yastij