-
Notifications
You must be signed in to change notification settings - Fork 2.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
Add API section to Reference Section #5313
Conversation
This PR has multiple commits, and the default merge method is: merge. 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. |
/assign |
@ncapps Could you review this when you get the chance? |
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.
Minor comments regarding links.
Compose kustomizations. | ||
--- | ||
|
||
Please check out the existing [components guide](/guides/config_management/components/) for explanation with examples. |
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.
Suggest updating this link path to /docs/concepts/components/
.
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 added this content instead of referencing it because I don't think that the page will be added to our new website
I also added the reference to the /docs/concepts/components/
page as you suggested.
9d126f6#diff-b6a9964b549b4e0db09ed4bebe9050677517c11321bf3a4dc754aec15e319e68R20
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.
As mentioned in this #5313 (review), because this Components page is going to eventually live under Concepts, can we remove the extra "and the components concept page"?
|
||
| Field | Required| Description | Default | | ||
| -----------: | :----:| ----------- | ---- | | ||
| `source`| :heavy_check_mark: | The source of the value | |
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.
The :heavy_check_mark:
doesn't appear to render. I am not sure if this is an issue with how Hugo is configured. This suggests we might be able to simply paste the emoji. Alternatively, we could use a different character or format to indicate that a field is required.
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.
Maybe we should apply changes in this PR. This one's not approved but it fixes the emoji issue
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.
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.
Nice catch! I looked into this issue briefly. I think the current approach of using the character works.
It seems that the original author tried to use Github syntax for emojis. We should be able to render characters like check marks also in html with Docsy shortcodes or Docsy's integration with Latex.
8788346
to
bd8045b
Compare
@ncapps What do you think is the better approach. |
I think that we can merge this PR as is and then improve the sections as more content is added to the site. |
/lgtm |
@ncapps: changing LGTM is restricted to collaborators In response to this:
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. |
/lgtm Thanks @ncapps and @bugoverdose! Quick note on my lgtm, I didn't actually look at the PR but wanted to mark the lgtm from @ncapps above |
I just found out the "kustomization_file.md" file that was added from other PR. Tell me if this what you intended. @annasong20 |
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.
@bugoverdose Thank you for migrating the fields in the Reference section!
I didn't clarify in the excel sheet and #5279 that unlike the previous site, we want to limit the new Reference section to field syntax like #5247 and move all other descriptions, examples, etc. into Concepts. This is completely my fault and I apologize.
Therefore, we can continue with the field descriptions in the Reference section in this PR. I created a separate issue #5341 to move this content. I agree with you and @ncapps that we can take an iterative approach to the documentation. I left some minor comments to update legacy documentation. Will approve once they're addressed.
/lgtm
@@ -0,0 +1,24 @@ | |||
--- |
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.
nit: Can we move deprecated fields into their own folder?
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.
Since the number of deprecated fields will increase as time goes, I think we should have a really clear rule/guideline on how we should organize deprecated fields and state it somewhere before applying it.
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.
Sure! How would you like to organize them?
Would you like to open a PR on this in a Documentation Style section on the new site? For reference, said section exists
- on the current site: https://kubectl.docs.kubernetes.io/contributing/docs/
- on the new site: https://kubernetes.io/docs/contribute/style/
## Helm Chart Inflation Generator | ||
|
||
Kustomize has limited support for helm chart inflation through the `helmCharts` field. | ||
You can read a detailed description of this field in the docs about [kustomize builtins]. |
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 link to the new site here. Also, as explained in the excel sheet, we don't want to use the confusing "builtins" terminology anymore.
Can you create a new empty "HelmChartInflationGenerator.md" under the Transformers directory to link on this page? We can replace "kustomize builtins" with "HelmChartInflationGenerator".
Helm chart inflation generator. | ||
--- | ||
|
||
[kustomize builtins]: https://kubectl.docs.kubernetes.io/references/kustomize/builtins/#_helmchartinflationgenerator_ |
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.
The new link suggested in the previous comment would live here.
title: "Components" | ||
linkTitle: "Components" |
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.
nit: Can we use lower case "components" here to match the other fields?
particular objects as specified by kustomize's | ||
configuration data. | ||
|
||
The default config data for vars is at [/api/konfig/builtinpluginconsts/varreference.go](https://github.com/kubernetes-sigs/kustomize/blob/master/api/konfig/builtinpluginconsts/varreference.go) |
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.
Can we replace this broken link with https://github.com/kubernetes-sigs/kustomize/blob/kustomize/v5.1.1/api/konfig/builtinpluginconsts/varreference.go
We recently moved the location of these constants. This new link also references a specific branch instead of master, so technically it should never break.
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: annasong20, bugoverdose 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 |
@bugoverdose The "approve" in my last comment must have triggered an actual approval... My comments weren't major. You can address them in a follow-up PR. |
closes #5279
This PR simply migrates all the existing documentation of Kustomization fields.