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(rendered): Pass helm legacy deploy flags to render config #9682

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

idsulik
Copy link
Contributor

@idsulik idsulik commented Jan 25, 2025

Fixes: #9681

Description
The helm's flag 'global' says additional flags passed on every command https://skaffold.dev/docs/references/yaml/?version=v4beta12#manifests-helm-flags-global, but skaffold doesn't pass while rendering, fixed it

@idsulik idsulik requested a review from a team as a code owner January 25, 2025 05:48
@idsulik idsulik requested a review from menahyouyeah January 25, 2025 05:48
Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

Hi team,

I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly understand the changes:

This pull request addresses issue #9681. The goal is to fix an issue where Skaffold wasn't passing legacy Helm deploy flags during the rendering process. Specifically, the global flag in Helm, which is intended to pass additional flags to every command, was not being correctly handled.

Changes:

  • The primary change is in pkg/skaffold/runner/renderer.go. The code was modified to include the p.Deploy.LegacyHelmDeploy.Flags in the latest.RenderConfig for Helm. This ensures that legacy Helm deploy flags are now passed correctly during rendering. The relevant lines are 67-73, where the Flags field was added to the latest.Helm struct within the RenderConfig.
--- a/pkg/skaffold/runner/renderer.go
+++ b/pkg/skaffold/runner/renderer.go
@@ -67,6 +67,7 @@ func GetRenderer(ctx context.Context, runCtx *runcontext.RunContext, hydrationDi
 			rCfg := latest.RenderConfig{
 				Generate: latest.Generate{
 					Helm: &latest.Helm{
+						Flags: p.Deploy.LegacyHelmDeploy.Flags,
 						Releases: legacyHelmReleases,
 					},
 				},

In short: This PR ensures that Skaffold correctly handles legacy Helm deploy flags, fixing a discrepancy in how the global flag was being applied during the rendering stage.


A little haiku to lighten the mood:

Flags now take flight,
Rendering's path made clear and bright,
Skaffold's code takes flight.

Copy link

@code-review-assist code-review-assist bot left a comment

Choose a reason for hiding this comment

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

This pull request addresses issue #9681 by passing Helm legacy deploy flags to the render configuration. This ensures that global flags specified in the skaffold.yaml are also applied during the rendering process. The change looks correct and straightforward. However, it would be beneficial to add a unit test to verify the change and update the documentation to reflect this new behavior.

I've summarized the style guide elements I referenced below:

  • Go Code Review Comments: While there wasn't a specific style guide provided, I referenced general Go best practices and the Effective Go document when suggesting improvements.

I recommend addressing the documentation and testing suggestions before merging this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

skaffold render --debug suggested but missing
1 participant