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

New flag --document-dependency-values #112

Merged
merged 5 commits into from
Apr 25, 2022

Conversation

armsnyder
Copy link
Contributor

@armsnyder armsnyder commented Oct 4, 2021

Features

Adds new CLI flag -u/--document-dependency-values for documenting values from dependencies, recursively.

The flag is disabled by default, so it should not affect existing behavior.

Relates to #70 ; however it only supports local dependencies for now since it relies on the existing parsing logic.

Bug fixes

This change also includes two drive-by bug fixes.

  • Replace usages of path.Join with filepath.Join when joining directories, for better cross-platform support.
  • Fix a FileSortOrder sorting bug in model.go where it would not sort by line due to an incorrect valuesTableRows[i].LineNumber < valuesTableRows[i].LineNumber comparison operation.

Details

I added a new example chart called umbrella and ran the helm-docs command on it using --document-dependency-values to illustrate the change.

In main.go, instead of parsing and printing on the same pass, I changed it to parse first and print second. This let me reference values files from dependencies which were already parsed. Both passes use parallel processing, as before.

@armsnyder
Copy link
Contributor Author

@norwoodj My team has been battle-testing this change in our medium-size Helm chart repositories over the past month and are following my fork for now. No rush getting this reviewed, but I thought you should know it works in production.

@LtChae
Copy link

LtChae commented Nov 24, 2021

My team has umbrella charts as well that would benefit from this change. Definitely interested in seeing this merged.

@dirien
Copy link
Contributor

dirien commented Mar 1, 2022

Thats cool! I think about to ask @norwoodj to allow additional maintainers to help to improve this project.

@norwoodj
Copy link
Owner

norwoodj commented Apr 3, 2022

I apologize for my slowness, I have not been very good at keeping up with pull requests here. I will try to take a look soon.

@armsnyder
Copy link
Contributor Author

Thanks! I'll resolve conflicts on this today.

armsnyder and others added 5 commits April 3, 2022 14:05
Adds new CLI flag -u/--document-dependency-values.

The flag is disabled by default, so it should not affect existing behavior.

**This change also includes two drive-by bug fixes!**

* Replace usages of `path.Join` with `filepath.Join` when joining directories, for better cross-platform support.
* Fix a `FileSortOrder` sorting bug in `model.go` where it would not sort by line due to an incorrect `valuesTableRows[i].LineNumber < valuesTableRows[i].LineNumber` comparison operation.
When evaluating chart dependencies, log a warning instead of aborting dependency resolution, if a dependency does not exist because it was invalid. This is useful for library charts which may not have values.
@armsnyder
Copy link
Contributor Author

@norwoodj I rebased my branch. I saw that the PrintDocumentation function signature was modified in a breaking way recently. I assume we don't care about making breaking Go package changes, so I'm changing my own argument which was previously ...DependencyValues (non-breaking) to []DependencyValues (breaking).

@norwoodj norwoodj merged commit 902641c into norwoodj:master Apr 25, 2022
@norwoodj
Copy link
Owner

Merged. Thanks a ton for the contribution and sorry again for being so slow to review it.

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.

4 participants