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

Fix flyte-core rendering when flyteagent enabled #4922

Merged
merged 1 commit into from
Feb 26, 2024

Conversation

ddl-ebrown
Copy link
Contributor

  • Flyte agent now tries to render a specific set of podLabels, so assumes that there is an empty podLabels: {} set. This change to values.yaml for flyte-core ensures empty values are there.

  • Fixes:

    Error: template: flyte-core/charts/flyteagent/templates/agent/deployment.yaml:17:17: executing "flyte-core/charts/flyteagent/templates/agent/deployment.yaml" at <include "flyteagent.podLabels" .>: error calling include: template: flyte-core/templates/_helpers.tpl:122:16: executing "flyteagent.podLabels" at <.Values.flyteagent.podLabels>: nil pointer evaluating interface {}.podLabels

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

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@dosubot dosubot bot added size:XS This PR changes 0-9 lines, ignoring generated files. bug Something isn't working documentation Improvements or additions to documentation labels Feb 20, 2024
 - Flyte agent now tries to render a specific set of podLabels, so
   assumes that there is an empty `podLabels: {}` set. This change to
   values.yaml for flyte-core ensures empty values are there.

 - Fixes:

   Error: template: flyte-core/charts/flyteagent/templates/agent/deployment.yaml:17:17: executing "flyte-core/charts/flyteagent/templates/agent/deployment.yaml" at <include "flyteagent.podLabels" .>: error calling include: template: flyte-core/templates/_helpers.tpl:122:16: executing "flyteagent.podLabels" at <.Values.flyteagent.podLabels>: nil pointer evaluating interface {}.podLabels

Signed-off-by: ddl-ebrown <[email protected]>
@ddl-ebrown ddl-ebrown force-pushed the fix-flytagent-defaults branch from 181d8ea to f854f1e Compare February 20, 2024 21:14
Copy link

codecov bot commented Feb 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (e58e3d3) 58.92% compared to head (f854f1e) 58.91%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4922      +/-   ##
==========================================
- Coverage   58.92%   58.91%   -0.02%     
==========================================
  Files         645      645              
  Lines       55394    55394              
==========================================
- Hits        32640    32634       -6     
- Misses      20171    20177       +6     
  Partials     2583     2583              
Flag Coverage Δ
unittests 58.91% <ø> (-0.02%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@@ -279,6 +279,8 @@ flyteagent:
defaultAgent:
endpoint: "dns:///flyteagent.flyte.svc.cluster.local:8000"
insecure: true
# -- Labels for flyteagent pods
podLabels: {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @ddl-ebrown I remove podLabels from the helper in this PR.

Also, would you mind taking a look at the new agent API? We want to make it as simple as possible, so make some change to it. flyteorg/flytekit#2146

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you remove podLabels just as a workaround for the chart install issue? In general, I think we want to keep that functionality (I added it recently in #4756)

Will have a look at the agent changes - thanks!

Copy link
Member

@pingsutw pingsutw Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just as a workaround for the chart install issue

yes

I added it recently in #4756)

I see, got it. Let's merge your PR first.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wondering if there's any pending action on this PR?

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Feb 26, 2024
@davidmirror-ops davidmirror-ops merged commit d751d0b into flyteorg:master Feb 26, 2024
50 of 51 checks passed
yubofredwang pushed a commit to yubofredwang/flyte that referenced this pull request Mar 26, 2024
- Flyte agent now tries to render a specific set of podLabels, so
   assumes that there is an empty `podLabels: {}` set. This change to
   values.yaml for flyte-core ensures empty values are there.

 - Fixes:

   Error: template: flyte-core/charts/flyteagent/templates/agent/deployment.yaml:17:17: executing "flyte-core/charts/flyteagent/templates/agent/deployment.yaml" at <include "flyteagent.podLabels" .>: error calling include: template: flyte-core/templates/_helpers.tpl:122:16: executing "flyteagent.podLabels" at <.Values.flyteagent.podLabels>: nil pointer evaluating interface {}.podLabels

Signed-off-by: ddl-ebrown <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working documentation Improvements or additions to documentation lgtm This PR has been approved by a maintainer size:XS This PR changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants