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

bug: HelmState.scatterGather ignores item parameter #793

Closed
travisgroth opened this issue Aug 7, 2019 · 1 comment · Fixed by #798
Closed

bug: HelmState.scatterGather ignores item parameter #793

travisgroth opened this issue Aug 7, 2019 · 1 comment · Fixed by #798
Labels

Comments

@travisgroth
Copy link
Contributor

Came across this in #782

See

numReleases := len(st.Releases)
if concurrency < 1 {
concurrency = numReleases
} else if concurrency > numReleases {
concurrency = numReleases
}

It looks like the intention was to make that method generic and be informed of the number of items coming in, but it seems to default/max against the number of releases instead.

This may be the first/only non-release based usage. It would be good to re-use it for environment secrets.

My proposal is to use the items parameter, which is currently unused. Can put in a PR if that makes sense.

@mumoshu
Copy link
Collaborator

mumoshu commented Aug 7, 2019

@travisgroth Wow! Good catch.

Yeah items should be used instead of len(st.Releases) when concurrency isn't specified(0).

To be extra clear, I believe it should be:

 if concurrency < 1 { 
 	concurrency = items 
 } else if concurrency > items { 
 	concurrency = items 
 } 

Or even just:

 if concurrency < 1 || concurrency > items { 
 	concurrency = items 
 }

Btw concurrency > items is there to cap concurrency to number of items. This isn't strictly needed, but for avoiding unnecessary/redundant goroutines.

I'd appreciate it if you could submit a 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 a pull request may close this issue.

2 participants