-
Notifications
You must be signed in to change notification settings - Fork 670
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
Fix webhook typo, add podLabels, add podEnv to flyte-core Helm chart #4756
Conversation
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
Signed-off-by: ddl-ebrown <[email protected]>
36bf642
to
392e02b
Compare
I tested setting the podLabels / podEnv, ran |
392e02b
to
b77c0f6
Compare
- podLabels is a standard pod value often needed to introduce user-customizable labels to pods Signed-off-by: ddl-ebrown <[email protected]>
- podEnv is a standard Helm addition used to add additional environment variables to pods - NOTE: flyteadmin already defined a `env: []`, so no podEnv was added there was it would be a breaking change Signed-off-by: ddl-ebrown <[email protected]>
b77c0f6
to
174dd91
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4756 +/- ##
==========================================
+ Coverage 58.17% 58.18% +0.01%
==========================================
Files 626 626
Lines 53833 53833
==========================================
+ Hits 31315 31322 +7
+ Misses 20010 20003 -7
Partials 2508 2508
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
@davidmirror-ops any chance you would be able to have a look at this one? Thanks! |
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.
Thanks @ddl-ebrown!
This is very helpful not only for consistent config but eventually even for observability use cases I think. I'm also curious about the actual use case that led your org to standardize on these keys.
BTW, please consider signing up for the weekly community raffle, this contribution would give you points!
Cool thanks @davidmirror-ops! We use annotations specifically for things like Istio and Prometheus. Clearly, they're useful for many different operators. We primarily use labels for configuring network policy. So pods marked with
|
- In flyteorg#4756 / fb9ffd5, flyte-core got consistent podEnv values established in values.yaml. However, these values were not properly injected into *all* the containers being used in various deployments. Fix that so that they are used in all deployments Signed-off-by: ddl-ebrown <[email protected]>
* Flyte-core add missing podEnv values - In #4756 / fb9ffd5, flyte-core got consistent podEnv values established in values.yaml. However, these values were not properly injected into *all* the containers being used in various deployments. Fix that so that they are used in all deployments Signed-off-by: ddl-ebrown <[email protected]> * Flyte-core chart prevent empty env: - Some linters consider empty env: as invalid k8s YAML, because env is typically an [] when no values are set Prevent rendering the console env block without values Signed-off-by: ddl-ebrown <[email protected]> --------- Signed-off-by: ddl-ebrown <[email protected]>
- Previously, the webhook was sharing some pod level settings in the core chart with flytepropeller like: * podAnnotations * podEnv * podLabels * nodeSelector Since the webhook runs a separate pod, it should have separate settings NOTE: no attempt is made to honor carrying over any previous settings from flytepropeller values to webhook values, but given these were recently introduced / fixed in January as part of flyteorg#4756, I think they're not used very much Signed-off-by: ddl-ebrown <[email protected]>
Tracking issue
https://github.com/flyteorg/flyte/issues/
Why are the changes needed?
Hopefully this isn't too controversial a change. To deploy in our environments, we've standardized on the use of
podAnnotations
,podLabels
andpodEnv
helm values. Globally, this allows for setting values across all deployment, statefulset and daemonset resources.What changes were proposed in this pull request?
Flyte already has
podAnnotations
, but notpodLabels
orpodEnv
. This PR adds both toflyte-core
.Of not,
flyteadmin
has aenv: []
value set already, but it's the only one. I didn't want to break anything so left it as-is. Ideally I think that would becomepodEnv: {}
and would render in the same way as the other charts.How was this patch tested?
make helm
was used locally to generate charts / docs. I set values explicitly in thevalues.yaml
to verify correct renderingi.e.
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link