-
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
📖 Update optional fields in docs for Machine and Cluster controllers #7328
Conversation
/lgtm Content looks accurate based on the code. Not related to this pr but a general question. How detailed should the documentation be? When it becomes this detailed it's getting very close to just reading the code or the godoc. Comparing:
with https://pkg.go.dev/sigs.k8s.io/[email protected]/api/v1alpha4#FailureDomainSpec |
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.
/lgtm
IMO having these definitions be close to the godoc isn't necessarily a bad thing as the godoc isn't a great source of information for the contract definition.
I think these pages are close to the source of truth for the reality of the provider contracts, so having field information that's close to the level of detail of the godoc isn't IMO an issue.
/lgtm |
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.
Sorry, last nits from my side. Just noticed that the current definition of addresses are not entirely correct
@Jont828 Could you please also squash your commits? If you don't know how it's described in CONTRIBUTING |
3ccbf3e
to
20ed1a1
Compare
@oscr Done, let me know if there's anything else I need to do to get this merged! |
Thank you very much! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sbueringer 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 |
What this PR does / why we need it: Update optional fields in docs for Machine and Cluster controllers
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 #7300 #7301