feat: added a new Helm option ignoreMissingValueFiles (#7767) #8003
Conversation
Codecov Report
@@ Coverage Diff @@
## master #8003 +/- ##
=======================================
Coverage 41.54% 41.54%
=======================================
Files 174 174
Lines 22681 22696 +15
=======================================
+ Hits 9422 9430 +8
- Misses 11918 11925 +7
Partials 1341 1341
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Thank you for a useful feature @ocraviotto ! PR looks very solid. Noticed only one minor concern about the new warning message in logs. Let me know what you think please
reposerver/repository/repository.go
Outdated
|
|
||
| _, err = os.Stat(path) | ||
| if os.IsNotExist(err) { | ||
| log.Warnf(" %s values file does not exist", path) |
There was a problem hiding this comment.
Most of the warning messages in logs indicate a problem that Argo CD operators have to act on. I think a warning about a missing values file is intended for the end-user only. This warning message will be printed in logs pretty frequently and won't be very useful for operators.
I would suggest changing the log level to DEBUG. Additionally, we can introduce a "warning" condition instead. (similar to
). In my opinion "warning" condition is a nice to have a feature and I would work on it in a separate PR.There was a problem hiding this comment.
Thank you @alexmt for taking the time to start a review and for your comments.
I agree with the above on changing the log level as this definitely affects the end-user and not the operator. TBH I did think initially to emit an event for the application resource, but your pointing me to the status conditions as an alternative to warn the user makes sense too and I'll explore it (though might leave it for a separate PR depending on how much work it'd represent and other considerations, like the proper warning condition etc.).
In any case, my first idea was to let the end-user know about missing files, yet when looking at where in the RepoServer the file check takes place and what the various methods invoked return, I opted for the simpler log approach without much thought on how this would affect logs for operators with multiple managed resources.
To sum up, I'll update my branch, modify the log level and the location of the log within the affirmative conditional below (in normal conditions the helm operation will fail and we should have information on the missing file(s) being the cause) and explore ways to update resource status conditions and at least leave a comment here on that.
There was a problem hiding this comment.
@alexmt after a quick look, it seems to me that the best way to pass back additional information (used for conditions or events associated with application resources) from the RepoServer would be to add a new field to the proto ManifestResponse message for the caller to handle the condition update or event generation based on that field value.
If this sounds reasonable, I'd be willing to take a stab at it. In that case, let me know if you'd like me to create an enhancement first or what would be the best way to proceed.
There was a problem hiding this comment.
I agree it makes sense to use ManifestResponse to pass back additional information. Thank you for resolving the comment about log level! Merging this PR.
|
Failed |
so as to allow operators to prevent Argo CD from passing valueFiles to helm template if they don't exist in the source under the specified path. Signed-off-by: Oscar Craviotto <craviotto@avellaneda.com>
ocraviotto
force-pushed
the
ignore-missing-value-files
branch
from
c1c5c9e to
7089bce
Compare
so as to allow operators to prevent Argo CD from passing valueFiles
to helm template if they don't exist in the source under the specified path.
Signed-off-by: Oscar Craviotto craviotto@avellaneda.com
Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist:
Closes #7767