-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: add buildDependenciesSkipRefresh option to HelmRelease to pass --skip-refresh to helm dep bulid #9672
base: main
Are you sure you want to change the base?
feat: add buildDependenciesSkipRefresh option to HelmRelease to pass --skip-refresh to helm dep bulid #9672
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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.
Hi team,
I'm currently reviewing this pull request and will provide my feedback shortly. In the meantime, here's a summary for you and other reviewers to quickly get up to speed:
This pull request, authored by art-shutter, adds a new configuration option buildDependenciesSkipRefresh
to the HelmRelease
section of the Skaffold configuration. The goal is to allow users to skip refreshing already built Helm dependencies when using helm dep build
. This addresses issue #8413.
Here's a breakdown of the changes:
- Added
buildDependenciesSkipRefresh
option: A new boolean field,buildDependenciesSkipRefresh
, has been added to theHelmRelease
struct inpkg/skaffold/schema/latest/config.go
(lines 1050-1055). This option defaults tofalse
. - Updated schema: The schema definition in
docs-v2/content/en/schemas/v4beta12.json
has been updated to reflect the new option (lines 2365-2376 and 2494-2506). - Implemented skip refresh logic: The
helm.go
file (pkg/skaffold/render/renderer/helm/helm.go
) now incorporates thebuildDependenciesSkipRefresh
option. If it's set totrue
, the--skip-refresh
flag is passed to thehelm dep build
command (lines 208-218). - Minor cleanup: Some minor formatting changes were made in
pkg/skaffold/schema/latest/config.go
(lines 1856-1865 and 1883-1891).
The changes are well-structured and easy to follow. The addition of the buildDependenciesSkipRefresh
option provides a valuable performance improvement for users with large Helm charts.
And now, a little haiku to lighten the mood:
Code flows like a stream,
New features gently emerge,
Reviews make it shine.
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.
This pull request adds a buildDependenciesSkipRefresh
option to the HelmRelease config in Skaffold. This allows users to skip refreshing Helm dependencies during builds, which can be useful for speeding up builds when dependencies haven't changed. The implementation looks good overall, and the documentation updates are clear. I've summarized the Google Go Style Guide elements I referenced below:
- Naming: Used for the new option name
BuildDependenciesSkipRefresh
. - Comments/Documentation: Ensured the new option is documented clearly in the schema and the code.
Here are a few minor suggestions for improvement:
I did sign a CLA, can we re-run the check, please? |
I reran it yesterday and today and looks like it's still failing |
@@ -2494,6 +2500,7 @@ | |||
"wait", | |||
"recreatePods", | |||
"skipBuildDependencies", | |||
"buildDependenciesSkipRefresh", |
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.
to stay consistent with the other flag "skipBuildDependencies", can you rename to "skipBuildDependenciesRefresh"? I had to look up what the other one was doing and with this rename I think it will be clearer what each flag is doing
if err := helm.ExecWithStdoutAndStderr(ctx, h, io.Discard, errBuffer, false, env, "dep", "build", release.ChartPath); err != nil { | ||
cmdArgs := []string{"dep", "build", release.ChartPath} | ||
if release.BuildDependenciesSkipRefresh { | ||
cmdArgs = []string{"dep", "build", "--skip-refresh", release.ChartPath} |
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.
instead of redefining, can you just += to cmdArgs slice? I.e. just add "--skip-refresh"? Does release.ChartPath have to be the last arg?
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.
It doesn't have to be last in helm cli. Changed it to append instead of recreate.
if err := helm.ExecWithStdoutAndStderr(ctx, h, io.Discard, errBuffer, false, env, "dep", "build", release.ChartPath); err != nil { | ||
cmdArgs := []string{"dep", "build", release.ChartPath} | ||
if release.BuildDependenciesSkipRefresh { | ||
cmdArgs = []string{"dep", "build", "--skip-refresh", release.ChartPath} |
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.
can you add a unit test for this?
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.
moved to its own func and added a unit test
pkg/skaffold/schema/latest/config.go
Outdated
@@ -1050,6 +1050,10 @@ type HelmRelease struct { | |||
// Ignored for `remoteChart`. | |||
SkipBuildDependencies bool `yaml:"skipBuildDependencies,omitempty"` | |||
|
|||
// BuildDependenciesSkipRefresh should the refresh of already built dependencies be skipped. |
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.
nit on grammar: determines whether the refresh of already built dependencies should be skipped. I'd also add that if it's set to true, the --skip-refresh flag is passed to the helm dep build command
…ping refresh of built dependencies
…esh and update related logic
db45528
to
e6a93b8
Compare
I amended the commits to use the email address that was used to sing the CLA. The check is passing now |
@@ -171,7 +179,7 @@ func (h Helm) generateHelmManifest(ctx context.Context, builds []graph.Artifact, | |||
return nil, helm.UserErr("cannot marshal overrides to create overrides values.yaml", err) | |||
} | |||
|
|||
if err := os.WriteFile(constants.HelmOverridesFilename, overrides, 0666); err != nil { | |||
if err := os.WriteFile(constants.HelmOverridesFilename, overrides, 0o666); err != nil { |
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.
just wondering why you changed this?
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.
it's octal permissions, added by gofumpt automatically. I didn't intend this change as this is not the accepted tool in the repo. Reverted.
Would this be better implemented as new (edited to add:)
This means if there's some other flag another user wants in the future, they could immediately use it without adding yet another configuration option to the skaffold.yaml. Curious if you have thoughts on this? (also @idsulik) |
Yes, I think it'll be better to avoid adding more and more flags into the skaffold's config, we should follow the install/upgrade flags and add
|
here it is: #9696 |
Fixes: #8413
Description
This adds a new configuration option
buildDependenciesSkipRefresh
to skaffold render config.User facing changes
Users can now supply a new option in the helm config for skaffold render when using helm as a renderer