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

Update RKE2 config ref YAML options #4086

Merged
merged 12 commits into from
Jun 21, 2022
Merged

Update RKE2 config ref YAML options #4086

merged 12 commits into from
Jun 21, 2022

Conversation

btat
Copy link
Contributor

@btat btat commented May 10, 2022

No description provided.

@btat btat changed the base branch from master to staging May 10, 2022 01:21
@btat btat requested review from sowmyav27 and thedadams May 11, 2022 22:19
Copy link
Contributor

@snasovich snasovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some suggestions.
Would appreciate for @thedadams to review my comments as I'm not exactly solid on their correctness.

key: value
```

This would install the chart with `chart-name` with the values `key = value`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This statement gives the impression that we specify which chart to install which I don't believe is the case. E.g. if no chartValues are specified, RKE2 still installs whatever charts it needs; this section is purely to allow customizing values for the charts RKE2 installs (either by default or based on other settings passed - e.g. machineGlobalConfig.cni sets which CNI is to be installed and related Helm chart is installed automatically).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically speaking, I believe it does mean that Rancher will ask RKE2 to install the chart. The issue is whether or not RKE2 knows about the chart. If RKE2 doesn't know about the chart, then Rancher asking it to install it won't work.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the clarification here. If we make the suggested change few lines above, I think this can be left as is.
On a side note, I'm wondering if there is a way to reference a comprehensive list of charts coming from RKE2?
I couldn't find it documented in RKE2 docs.
@cwayne18 @brandond @briandowns

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this does enable the chart. It just creates a HelmChartConfig for the chart with the matching name, which will provide values to that chart IF it is enabled. I would love to have this confirmed though.

The RKE2 bundled charts live in the rke2-charts repo, and the versions packaged in a given release can be found in the Dockerfile. Some of these, such as the cloud-provider and CNI charts, only have one active at a time. We don't document this anywhere else, at the moment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would the consensus be to remove the line for now and add back any additional info once it's been confirmed?

@snasovich @thedadams @brandond

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, let's just remove this line - the documentation is fine without it.

btat and others added 2 commits May 11, 2022 19:22
Co-authored-by: Sergey Nasovich <85187633+snasovich@users.noreply.github.com>
@btat btat requested review from snasovich and thedadams May 12, 2022 03:49
key: value
```

This would install the chart with `chart-name` with the values `key = value`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this does enable the chart. It just creates a HelmChartConfig for the chart with the matching name, which will provide values to that chart IF it is enabled. I would love to have this confirmed though.

The RKE2 bundled charts live in the rke2-charts repo, and the versions packaged in a given release can be found in the Dockerfile. Some of these, such as the cloud-provider and CNI charts, only have one active at a time. We don't document this anywhere else, at the moment.

btat and others added 2 commits May 12, 2022 11:08
Copy link
Contributor

@snasovich snasovich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor updates requested, otherwise LGTM

btat and others added 4 commits May 13, 2022 13:24
…nfig-reference/_index.md

Co-authored-by: Sergey Nasovich <85187633+snasovich@users.noreply.github.com>
…nfig-reference/_index.md

Co-authored-by: Sergey Nasovich <85187633+snasovich@users.noreply.github.com>
…nfig-reference/_index.md

Co-authored-by: Brad Davidson <brad@oatmail.org>
@btat btat dismissed stale reviews from snasovich, brandond, and sowmyav27 June 21, 2022 22:15

Feedback addressed

@btat btat merged commit 08281f2 into rancher:staging Jun 21, 2022
@btat btat deleted the rke2-yaml branch June 21, 2022 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants