fix rendering of flyte-core and flyteagent charts #5048
Conversation
|
Thank you for opening this pull request! 🙌 These tips will help get your PR across the finish line:
|
dosubot
bot
added
size:S
Signed-off-by: Patrick Brogan <pbrogan12@gmail.com>
pbrogan12
force-pushed
the
fix-helm-flyteagent
branch
from
0fdf967
to
80c0c5b
Compare
There was a problem hiding this comment.
Thanks for fixing it. cc @ddl-ebrown did you run into this issue?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5048 +/- ##
==========================================
- Coverage 59.00% 58.99% -0.02%
==========================================
Files 645 645
Lines 55578 55578
==========================================
- Hits 32792 32786 -6
- Misses 20194 20200 +6
Partials 2592 2592
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
@pingsutw I encountered this issue when trying to upgrade to v1.11.0 too. |
|
@yini7777 did you enable the flyte agent as well? |
|
@pingsutw Yes, I did enable the flyteagent. |
|
generate_helm workflow failing due to the spark-operator helm repo not being not available. |
yes, please do it, thanks! |
dosubot
bot
added
size:M
Signed-off-by: Patrick Brogan <pbrogan12@gmail.com>
pbrogan12
force-pushed
the
fix-helm-flyteagent
branch
from
539d6cc
to
edc72ed
Compare
No we haven't run across this issue that I'm aware of - we're using the charts as-is without any issues right now. We are running forks of the charts internally and:
I'll run through an upgrade to 1.11 shortly and report back |
|
I should also note that the last published From a consumer standpoint that needs to install multiple agents, being able to use a single parameterized agent chart that ships standalone is easier IMHO. I'll also note that we use Helmfile -- so by having a standalone Flyte agent chart that's configurable, it's very easy to install multiple releases of the chart that each correspond to a different agent. UPDATE: I see that the chart with |
| @@ -31,7 +31,7 @@ app.kubernetes.io/managed-by: {{ .Release.Service }} | |||
| {{- define "flyteagent.podLabels" -}} | |||
| {{ include "flyteagent.labels" . }} | |||
| {{- with .Values.podLabels }} | |||
| {{- toYaml . }} | |||
| {{ toYaml . }} | |||
There was a problem hiding this comment.
👍 I can confirm that this change is correct
This is what I have in our local copy of the chart, so it would appear that I accidentally did not carry it over into my PR at #4756 (fb9ffd5#diff-40d80930ba32a1baa6eaca2a0694d9b0d21b8d72fc71fd71405b2973fde1a5aaR34)
Apologies!
There was a problem hiding this comment.
Will a new version of the chart be published before the 1.12 release w/ these fixes?
There was a problem hiding this comment.
we can certainly have this out as a patch release before 1.12.
|
Congrats on merging your first pull request! 🎉 |
While testing out v1.11.0 ran into a couple of issues when enabling flyteagent from the flyte-core chart.
We define the templates
flyteagent.*in both flyte-core and flyteagent charts.Since template names are global across top level charts/subcharts, I ran into an issue dereferencing podValues.
In this case it was using the flyte-core definition of the template instead of flyteagent's.
There is also a small typo in the flyteagent template
flyteagent.podLabels, which results in the flyteagent chart not being able to render if podValues is set.Tracking issue
https://github.com/flyteorg/flyte/issues/
Why are the changes needed?
What changes were proposed in this pull request?
How was this patch tested?
Setup process
Screenshots
Check all the applicable boxes
Related PRs
Docs link