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

Document zeebe values #141

Merged
merged 7 commits into from
Jan 31, 2022
Merged

Document zeebe values #141

merged 7 commits into from
Jan 31, 2022

Conversation

Zelldon
Copy link
Member

@Zelldon Zelldon commented Jan 31, 2022

Adds the zeebe values to the parent values file and removes them from the old values file, to reduce redundancy. Document all zeebe related variables to follow best practices https://helm.sh/docs/chart_best_practices/values/

This means:

  • Variable names should begin with a lowercase letter, and words should be separated with camelcase.
  • Every defined property in values.yaml should be documented. The documentation string should begin with the name of the property that it describes, and then give at least a one-sentence description

Furthermore introduced own pattern: # [VarName] [conjunction] [definition]

VarName:

  • In the documentation the variable name is started with a big letter, similar to kubernetes resource documentation.
  • If the variable is part of a subsection/object we use a json path expression (to make it more clear where the variable belongs to).
    The root (chart name) is omitted (e.g. zeebe). This is useful for using --set in helm.

Conjunction:

  • [defines] for mandatory configuration
  • [can be used] for optional configuration
  • [if true] for toggles
  • [configuration] for section/group of variables

Need to adjust the README related to the Zeebe values

Rename JavaOpts -> to javaOpts, to follow naming convetion.
Rename create -> enabled to be consistent with other property names
Move probePath to correct section, readinessProbe to make it more clear
Rename created -> enabled to make it consistent
Add all zeebe related values to the ccsm-helm value file and document each of them.
Remove values from sub chart, they are overwritten by parent chart.
@Zelldon Zelldon requested a review from npepinpe January 31, 2022 07:43
Copy link
Member

@npepinpe npepinpe left a comment

Choose a reason for hiding this comment

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

Great stuff! The conventions are something we could add in a contributing section for the future contributors. That should hopefully avoid some review back and forth in the future.

  • ❌ The README for ccsm-helm still lists zeebe.JavaOpts and not zeebe.javaOpts as variable. Same for the zeebe sub-chart README. Though if you want to do that as part of adjusting the README in a separate PR, go ahead 🙂
  • ❓ You mention extracting the values to the parent chart; can you explain why? What's the plan, will all sub chart have their values in the parent/umbrella chart? Why is this a better alternative?

Anyway, no need for a second review from my side 👍

charts/ccsm-helm/values.yaml Show resolved Hide resolved
@Zelldon
Copy link
Member Author

Zelldon commented Jan 31, 2022

Thanks for your fast review @npepinpe !

Regarding:

You mention extracting the values to the parent chart; can you explain why? What's the plan, will all sub chart have their values in the parent/umbrella chart? Why is this a better alternative?

Multiple things, one is to avoid the duplication (having multiple values files with the same content), which some times lead to problems. Furthermore I would like to have on single place to maintain. Additionally artifacthub also shows only the values file of the parent chart https://artifacthub.io/packages/helm/camunda-cloud-helm/ccsm-helm?modal=values which makes for me sense to add the values here

@Zelldon Zelldon merged commit a31f7e4 into main Jan 31, 2022
@Zelldon Zelldon deleted the zell-document-zeebe-values branch January 31, 2022 10:07
@Zelldon Zelldon mentioned this pull request Jan 31, 2022
25 tasks
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.

None yet

2 participants