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

Add extra env vars to charts #7183

Merged
merged 12 commits into from Dec 4, 2023
Merged

Conversation

filintod
Copy link
Contributor

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:

@filintod filintod requested review from a team as code owners November 14, 2023 20:07
Copy link

codecov bot commented Nov 14, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (c11465f) 64.53% compared to head (9878e75) 64.55%.

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.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@ItalyPaleAle ItalyPaleAle left a 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: []
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

@filintod filintod Nov 15, 2023

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

Copy link
Contributor Author

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

filintod and others added 3 commits November 14, 2023 21:56
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>
@filintod
Copy link
Contributor Author

filintod commented Dec 1, 2023

@ItalyPaleAle could you take another look please

@artursouza
Copy link
Member

cc @ItalyPaleAle

@yaron2 yaron2 merged commit 5050df5 into dapr:master Dec 4, 2023
20 of 22 checks passed
DeepanshuA pushed a commit to DeepanshuA/dapr that referenced this pull request Dec 11, 2023
* 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>
JoshVanL pushed a commit to JoshVanL/dapr that referenced this pull request Dec 14, 2023
* 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>
@JoshVanL JoshVanL added this to the v1.13 milestone Feb 12, 2024
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.

Add ability to add custom environment variable to dapr control plane
6 participants