Feature/helm deploy #189
Feature/helm deploy #189
Conversation
Codecov Report
@@ Coverage Diff @@
## main #189 +/- ##
===========================================
- Coverage 100.00% 99.64% -0.36%
===========================================
Files 76 77 +1
Lines 3109 3136 +27
===========================================
+ Hits 3109 3125 +16
- Misses 0 11 +11
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| with open(helm_output_file_path, 'w') as helm_output_file: | ||
| sh.helm( # pylint: disable=no-member | ||
| 'secrets', 'upgrade', '--install', | ||
| self.get_value('helm-chart'), |
There was a problem hiding this comment.
@gfulton-redhat we should auto generate the helm chart name based on the org/app/service/enviornment names that are given. if you want you can allow someone to overwrite the auto generated name with a config value, but i dont see the use case at this time, but im sure someone will want to do it for some reason, but lets have a sensable default.
There was a problem hiding this comment.
I'm not too familiar with Helm, what's a reasonable format for the chart name?
Also, do we have a convention for handling default values?
There was a problem hiding this comment.
@GregJohnStewart Do you mean format in the sense of how it's spelled/capitalized/puncuated? Check out the best practices section of the upstream doc. Can you elaborate on the question about convention? Not sure I understand.
| sh.helm( # pylint: disable=no-member | ||
| 'secrets', 'upgrade', '--install', | ||
| self.get_value('helm-chart'), | ||
| self.get_value('helm-release'), |
There was a problem hiding this comment.
i think we can't just take helm-release as a paramter because i dont beleive you can pass a git repo as a target helm chart can you?
| 'secrets', 'upgrade', '--install', | ||
| self.get_value('helm-chart'), | ||
| self.get_value('helm-release'), | ||
| *self.get_value('helm-flags'), |
There was a problem hiding this comment.
to be consistent with maven can we call this helm-additional-arguments? (https://github.com/ploigos/ploigos-step-runner/blob/main/src/ploigos_step_runner/step_implementers/shared/maven_generic.py#L21)
There was a problem hiding this comment.
Anywhere else this would need to be changed?
There was a problem hiding this comment.
Not sure. You'll have to search the code. I'd assume there's at least one unit test where it matters.
|
Closing as stale. Feel free to reopen this if you want to work on it. |
No description provided.