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

doc: added values README via helm-docs cli #837

Merged
merged 19 commits into from
May 16, 2023

Conversation

justmike1
Copy link
Contributor

@justmike1 justmike1 commented Apr 25, 2023

What does this PR do?

Added README file to possess all values that traefik's helm chart reads
there are still improvements that will be done, like giving descriptions from the yaml's comments and optional values... but one PR at a time

Motivation

I checked the full value path I needed to write and reading from the yaml is difficult, for example ports is a long tree.

More

  • Yes, I updated the tests accordingly
  • Yes, I ran make test and all the tests passed

@mloiseleur
Copy link
Contributor

Hello @justmike1,

Thanks for your interests in Traefik and for this PR !
This can definitely help when using this chart.

I have some comments:

  1. Wdyt about adding a Makefile target to re-generate this file?
  2. There are a lot of parameters, so maybe we should keep this doc inside a specific file (like VALUES.md), in order to keep readability
  3. README.md is copied to traefik/ in order to be displayed in artifacthub page. See here.

@justmike1
Copy link
Contributor Author

@mloiseleur

  1. Added the Makefile target
  2. You can't change the name of the file the helm-docs output. so I added renaming via Makefile
  3. Changing to VALUES.md mitigates the issue, though in the future, when we will add also proper descriptions and default values via helm-docs supported comments and have a descriptive file, we will need to think a way to merge them sensibly and not just one on top of the other.

I use traefik for a year already and I don't see it change any time soon, so I will improve it with time.

@justmike1
Copy link
Contributor Author

I also added a test step in order for the file to stay updated upon PRs

@mloiseleur
Copy link
Contributor

mloiseleur commented Apr 26, 2023

That's better. I tried it with fb5eb41 by adding the needed -- into values.yaml for two first section and it looks good.
I guess we'll need to find how to properly display examples provided directly into values.yaml.

@ldez ldez changed the title feat(helm): added values README via helm-docs cli doc: added values README via helm-docs cli Apr 26, 2023
@justmike1
Copy link
Contributor Author

justmike1 commented Apr 26, 2023

@mloiseleur anything more required to push this PR? I don't like big PRs, so I would suggest steps:

  1. merging this PR
  2. PR for refactoring values.yaml comments to helm-docs
  3. PR for main README to have those values as well with keeping it's structure, like injecting them before contributions header, in order to be seen also in artifacthub

Edit: or your style is having the entire feature 100% set in this PR?

@mloiseleur
Copy link
Contributor

I don't have a strong opinion of PR size.
I do have a strong opinion that we should not merge an empty VALUES.md.
I agree with you that it could be merged within the README.

=> For this PR, I think it's missing two things:

  1. Add the needed -- in values.yaml, to have a base on VALUES.md
  2. In Guidelines, non critical features are commented. How could those features be documented with this system ?

@justmike1
Copy link
Contributor Author

@mloiseleur Sorry for repeating commits, VALUES.md is full now, pending your next review.
Currently, I don't see a way in helm-docs to document commented out parameters, maybe in future versions, so I believe keeping it as is, and maybe we can layer the documentation, and then make a full one that will be structured from the files in CD flow.

@mloiseleur
Copy link
Contributor

I should be able to review it on next friday. I'll keep you informed.

Copy link
Contributor

@darkweaver87 darkweaver87 left a comment

Choose a reason for hiding this comment

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

LGTM, the lack of some features mis-documented by helm-docs tools it highlights the fact we have to refactor some parts of this helm chart.

@mloiseleur
Copy link
Contributor

This PR has brought a lot of questions.
It should help to make this chart simpler to apprehend.

Thanks again @justmike1 !

@traefiker traefiker merged commit 379f417 into traefik:master May 16, 2023
@justmike1
Copy link
Contributor Author

Thank you guys! @darkweaver87 @mloiseleur

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants