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

introduce helm-dependency-extra-args #396

Merged
merged 1 commit into from
Apr 5, 2022

Conversation

nazarewk
Copy link
Contributor

@nazarewk nazarewk commented Feb 18, 2022

What this PR does / why we need it:
TLDR; allows passing --skip-refresh to helm dependency build

Default behavior of helm dependency build is running equivalent of helm repo update before initiating packaging process.
This makes sense for handling a single repository - you always get the latest version even if you forget about running helm repo update yourself.

Passing --skip-refresh turns off the step allowing the user to run helm repo update once for the whole pipeline. The default behavior becomes redundant when handling a second chart in the same CI pipeline and becomes extremely wasteful when running build on large number of charts in a single pipeline.

ct lint invokes that default behavior for every single repository it checks, making it unnecessarily wasteful for it's primary use case. Sane default would be recommending ct users to invoke helm repo update before running ct lint, but that would be a breaking change.

This PR allows us to still invoke the proper behavior while preserving backwards compatiblity for ct users.

Which issue this PR fixes: fixes #368

Special notes for your reviewer:

Mocked test

I am not very proficient in mocking or Golang alltogether, but I believe TestLintYamlValidation works only by chance and my new TestLintDependencyExtraArgs contains proper invocation of Mock.On().

Basically I could not get the fakeMockLinter.AssertNumberOfCalls(t, "BuildDependenciesWithArgs", 1) to count occurences at all.

Submitted test is what I have determined to work empirically. When I ran the test twice with just a single .On() invocation it threw an error, invoking .On() followed by Repeatability = 1 allows a single call to the mocked method per test.

Introducing new method

I am not sure what is the project's external compatibility policy. I've added a fresh method to preserve backwards compatiblity when using ct as a library, since the struct is public.

@nazarewk nazarewk force-pushed the feature/dependencies-build-args branch 2 times, most recently from c3bca7c to 24b47f0 Compare February 18, 2022 15:08
@helm-bot helm-bot added size/M and removed size/L labels Feb 18, 2022
@nazarewk nazarewk force-pushed the feature/dependencies-build-args branch from 24b47f0 to 09f1778 Compare February 18, 2022 15:11
@cpanato
Copy link
Member

cpanato commented Mar 14, 2022

@nazarewk thanks for this PR, can you check the ci failures?

@nazarewk nazarewk force-pushed the feature/dependencies-build-args branch from 09f1778 to feb877b Compare March 14, 2022 10:57
@nazarewk

This comment was marked as outdated.

@nazarewk nazarewk force-pushed the feature/dependencies-build-args branch from feb877b to b71548c Compare March 14, 2022 14:50
@pull-request-size pull-request-size bot added size/L and removed size/M labels Mar 14, 2022
@nazarewk
Copy link
Contributor Author

nazarewk commented Mar 14, 2022

@cpanato This time it should pass (i've run go vet ./... && go test ./... locally), looks like it was not the right change before.

@cpanato cpanato requested a review from davidkarlsen March 19, 2022 09:12
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

@nazarewk thanks for the PR
can you generate the docs using the doc-gen? will be good as well to fetch the latest code from main

go build -o ct-bin ./ct/main.go

$ ./ct-bin doc-gen
Generating docs...
Done.

then commit the changes?
thanks!

addresses helm#368

Signed-off-by: Krzysztof Nazarewski <[email protected]>
@nazarewk nazarewk force-pushed the feature/dependencies-build-args branch from b71548c to 8e8e590 Compare March 23, 2022 10:11
@nazarewk
Copy link
Contributor Author

@cpanato docs are generated

@nazarewk nazarewk requested a review from cpanato March 28, 2022 07:02
@cpanato
Copy link
Member

cpanato commented Apr 1, 2022

@davidkarlsen can you take a look when you have some free cycle? thanks!

Copy link
Member

@davidkarlsen davidkarlsen left a comment

Choose a reason for hiding this comment

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

lgtm

@cpanato cpanato merged commit 3c014d8 into helm:main Apr 5, 2022
@nazarewk nazarewk deleted the feature/dependencies-build-args branch April 5, 2022 09:53
@cep21
Copy link

cep21 commented Apr 18, 2022

This is a great PR for our CI/CD system. Looking forward to getting it in a released version!

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.

Ability to add '--skip-refresh' to "helm dependency build"
5 participants