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

Cleanup Helm values #2767

Merged
merged 3 commits into from
Aug 15, 2023
Merged

Cleanup Helm values #2767

merged 3 commits into from
Aug 15, 2023

Conversation

clintyoshimura
Copy link
Contributor

Is there a related GitHub Issue?

#2650

What is this change about?

Cleanup the Helm values to make them more understandable for operators. The api.builderName parameter does not seem to be used and the yaml that Helm generated was the same after it was removed. The api.lifecycle.stagingRequirements parameter was moved to global.stagingRequirements because both api and controllers use those parameters.

Does this PR introduce a breaking change?

no

Acceptance Steps

All tests pass

Tag your pair, your PM, and/or team

@acosta11

Things to remember

  • Include any links to related PRs, issues, stories, slack discussions, etc... that will help establish context.
  • Is there anything else of note that the reviewers should know about this change?
  • This project follows the Cloud Foundry Code of Conduct
    -->

@clintyoshimura
Copy link
Contributor Author

In addition to the moved parameters, there were a few other issues that were not as significant that could use discussion.
adminUserName - This is a top level parameter. Should it be under global? All the other top level parameters are groups.
api.expose - Expose the API component via Contour. Should Contour be part of the parameter name?

@georgethebeatle
Copy link
Member

global is a special top level group that is visible to all subcharts in case your helm chart has subcharts. Here are the relevant docs. We used to have subcharts in the past and global is a historical leftover. It would be better if we put all common values such as stagingRequirements to the top level. We should also move the values under global a level up so that they are top level values too.

Copy link
Member

@georgethebeatle georgethebeatle left a comment

Choose a reason for hiding this comment

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

See above comment

Clint Yoshimura and others added 3 commits August 14, 2023 08:05
 - The api.builderName helm param is redundant
 - There is already a global.reconcilers.build param that is being used
   instead

[#2650]
 - Move the api.lifecycle.stagingRequirements to
   stagingRequirements because both api and controllers use those
   params
 - Leave the existing api.lifecycle params other than stagingRequirements
   because only api uses those

[#2650]

Co-authored-by: Matt Royal <mroyal@vmware.com>
 - Global is a historic leftover from when we had subcharts so the
   values in global should be at the top-level instead

[#2650]

Co-authored-by: Matt Royal <mroyal@vmware.com>
@clintyoshimura
Copy link
Contributor Author

@georgethebeatle We moved all the global helm values to the top-level and removed the global group. There will be korifi-ci changes required so when the PR is approved, we can push those changes to get the PR checks to pass and merge it that way. Everyone will have to rebase after that.

danail-branekov added a commit to cloudfoundry/korifi-ci that referenced this pull request Aug 15, 2023
See cloudfoundry/korifi#2767

Co-authored-by: Danail Branekov <danailster@gmail.com>
@danail-branekov danail-branekov merged commit a426011 into main Aug 15, 2023
7 checks passed
@danail-branekov danail-branekov deleted the cty-helm-values branch August 15, 2023 09:20
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.

3 participants