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
Add extra env vars to charts #7183
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #7183 +/- ##
==========================================
+ Coverage 64.53% 64.55% +0.02%
==========================================
Files 225 225
Lines 21070 21070
==========================================
+ Hits 13597 13602 +5
+ Misses 6306 6303 -3
+ Partials 1167 1165 -2 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please update the Helm chart README too
@@ -35,6 +35,8 @@ ports: | |||
|
|||
resources: {} | |||
|
|||
extraEnvVars: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make this a dictionary of key->value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually this is an array, at least in bitnami charts. I can see the benefit of using a dictionary, and thought about it, but that also includes adding a range to parse it into an array of dictionary in the template. I did not see env vars added in a chart in dapr that I looked for to follow so decided to follow bitnami's standard that is easier to implement.
I mentioned the format here #7182
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have other fields in the chart that allow setting annotations and are dictionaries.
Using an array allows doing things like referencing secrets. However secrets can't be mounted in the current Helm chart so it doesn't matter in this case.
Using a dictionary is a lot simpler to pass via --set
in Helm too.
My .02.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
annotations are dictionaries by default so it kind of make sense. true about --set
but now helm also has --set-json
that makes life easier for most of previous hard cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could add the dictionary. let me think of a way to not make it too bad/verbose in the template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ItalyPaleAle could you do another check
4d9ebb4
to
db074d8
Compare
Signed-off-by: Filinto Duran <filinto@diagrid.io>
Signed-off-by: Filinto Duran <filinto@diagrid.io>
Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: Filinto Duran <filinto@diagrid.io>
db074d8
to
a94aa76
Compare
@ItalyPaleAle could you take another look please |
* add extra env vars to charts Signed-off-by: Filinto Duran <filinto@diagrid.io> * use map instead of array, add docs to README Signed-off-by: Filinto Duran <filinto@diagrid.io> * Fix building on 32-bit systems (dapr#7185) Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: Filinto Duran <filinto@diagrid.io> --------- Signed-off-by: Filinto Duran <filinto@diagrid.io> Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: Loong Dai <long.dai@intel.com>
* add extra env vars to charts Signed-off-by: Filinto Duran <filinto@diagrid.io> * use map instead of array, add docs to README Signed-off-by: Filinto Duran <filinto@diagrid.io> * Fix building on 32-bit systems (dapr#7185) Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Signed-off-by: Filinto Duran <filinto@diagrid.io> --------- Signed-off-by: Filinto Duran <filinto@diagrid.io> Signed-off-by: ItalyPaleAle <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: Alessandro (Ale) Segala <43508+ItalyPaleAle@users.noreply.github.com> Co-authored-by: Loong Dai <long.dai@intel.com>
Description
Add optional extra env vars list to allow adding custom environment variables to dapr control plane system.
Issue reference
Please reference the issue this PR will close: #7182
Checklist
Please make sure you've completed the relevant tasks for this PR, out of the following list: