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

feat(helm): adding --name to update single repo #5182

Merged
merged 1 commit into from Jun 7, 2019

Conversation

thesattiraju
Copy link

@thesattiraju thesattiraju commented Jan 17, 2019

What this PR does / why we need it:
fixes #5127

Special notes for your reviewer:

If applicable:

  • this PR contains documentation
  • this PR contains unit tests
  • this PR has been tested for backwards compatibility

@helm-bot helm-bot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Jan 17, 2019
@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 17, 2019
@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 17, 2019
@helm-bot helm-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jan 24, 2019
@thesattiraju
Copy link
Author

@bacongobbler / @adamreese could you review this?

@adamreese
Copy link
Member

Rather than adding a --name flag, could we just use arguments?

Example: helm repo update stable

@thesattiraju
Copy link
Author

@adamreese I agree that removing --name makes it better.
We'll end up having one new path though right? i.e. helm repo update and helm repo update <repo_name>.
I was skeptical about how to convey that helm repo update meant helm would update all repos, so I chose this path instead.

@helm-bot helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 4, 2019
@thesattiraju
Copy link
Author

@adamreese I've updated it to helm repo update <repo_name> instead.

@thesattiraju
Copy link
Author

@adamreese @bacongobbler ping

@thesattiraju
Copy link
Author

@adamreese @bacongobbler pinging again

Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

@DS-MS Thanks for the changes. It is nearly there but it needs some updates as follows:

@thesattiraju
Copy link
Author

@hickeyma Updated the PR

Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

Thanks @DS-MS for the updates. I would appreciate if you could add a positive test as described in the inline comment.

@@ -132,3 +132,29 @@ func TestUpdateCmdStrictFlag(t *testing.T) {
t.Errorf("Expected 'Unable to get an update', got %q", got)
}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Do you mind adding a test that tests for a successful update with a single repo name?

Copy link
Author

Choose a reason for hiding this comment

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

@hickeyma added.

@helm-bot helm-bot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 7, 2019
@helm-bot helm-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 7, 2019
Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

Thanks for adding the positive test. Some nits on it. When you deliver changes, can you also squash the changes on this PR?

cmd/helm/repo_update_test.go Outdated Show resolved Hide resolved
cmd/helm/repo_update_test.go Outdated Show resolved Hide resolved
cmd/helm/repo_update_test.go Outdated Show resolved Hide resolved
cmd/helm/repo_update_test.go Outdated Show resolved Hide resolved
cmd/helm/repo_update_test.go Outdated Show resolved Hide resolved
cmd/helm/repo_update_test.go Outdated Show resolved Hide resolved
@thesattiraju thesattiraju force-pushed the RepoUpdateName branch 2 times, most recently from 0a94c1c to 15d5854 Compare June 7, 2019 09:49
@thesattiraju
Copy link
Author

@hickeyma updated

Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

Nearly there. Just small nit on caps on repo name for consistency.

@@ -53,12 +62,15 @@ func newRepoUpdateCmd(out io.Writer) *cobra.Command {
update: updateCharts,
}
cmd := &cobra.Command{
Use: "update",
Use: "update [repo_name]",
Copy link
Contributor

Choose a reason for hiding this comment

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

:s/repo_name/REPO_NAME
for consistency with commands like helm get, helm install etc.

Copy link
Author

Choose a reason for hiding this comment

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

done

Signed-off-by: Deepak Sattiraju <desattir@microsoft.com>

Lint

Signed-off-by: ds-ms <desattir@microsoft.com>

make docs

Signed-off-by: ds-ms <desattir@microsoft.com>

make docs

Signed-off-by: ds-ms <desattir@microsoft.com>
Signed-off-by: Deepak Sattiraju <desattir@microsoft.com>

using args instead of --name

Signed-off-by: ds-ms <desattir@microsoft.com>

Adding [repo_name] to use

Signed-off-by: ds-ms <desattir@microsoft.com>

Adding test

Signed-off-by: ds-ms <desattir@microsoft.com>

Adding positive test case

Signed-off-by: ds-ms <desattir@microsoft.com>

lint

Signed-off-by: ds-ms <desattir@microsoft.com>

Renaming

Signed-off-by: ds-ms <desattir@microsoft.com>

Updating repo_name to REPO_NAME

feat(helm): adding --name to update single repo

Signed-off-by: Deepak Sattiraju <desattir@microsoft.com>

Lint

Signed-off-by: ds-ms <desattir@microsoft.com>

make docs

Signed-off-by: ds-ms <desattir@microsoft.com>

make docs

Signed-off-by: ds-ms <desattir@microsoft.com>
Signed-off-by: Deepak Sattiraju <desattir@microsoft.com>

using args instead of --name

Signed-off-by: ds-ms <desattir@microsoft.com>

Adding [repo_name] to use

Signed-off-by: ds-ms <desattir@microsoft.com>

Adding test

Signed-off-by: ds-ms <desattir@microsoft.com>

Adding positive test case

Signed-off-by: ds-ms <desattir@microsoft.com>

lint

Signed-off-by: ds-ms <desattir@microsoft.com>

Renaming

Signed-off-by: ds-ms <desattir@microsoft.com>

Updating repo_name to REPO_NAME
Copy link
Contributor

@hickeyma hickeyma left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@hickeyma hickeyma merged commit 5133af0 into helm:master Jun 7, 2019
@thesattiraju thesattiraju deleted the RepoUpdateName branch June 7, 2019 12:03
marckhouzam added a commit to VilledeMontreal/helm that referenced this pull request Jun 22, 2021
This is a port to helm v3 of helm#5182.
A little more flexible than the v2 version, it allows to specify a list
of repositories that should be updated.

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
marckhouzam added a commit to VilledeMontreal/helm that referenced this pull request Jun 22, 2021
This is a port to helm v3 of helm#5182.
A little more flexible than the v2 version, it allows to specify a list
of repositories that should be updated.

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
marckhouzam added a commit that referenced this pull request Jul 8, 2021
This is a port to helm v3 of #5182.
A little more flexible than the v2 version, it allows to specify a list
of repositories that should be updated.

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
tylerauerbeck pushed a commit to tylerauerbeck/helm that referenced this pull request Aug 5, 2021
This is a port to helm v3 of helm#5182.
A little more flexible than the v2 version, it allows to specify a list
of repositories that should be updated.

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
zak905 pushed a commit to zak905/helm that referenced this pull request Jan 19, 2023
This is a port to helm v3 of helm#5182.
A little more flexible than the v2 version, it allows to specify a list
of repositories that should be updated.

Signed-off-by: Marc Khouzam <marc.khouzam@montreal.ca>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Needs v3 fix size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: add --repo-name to 'helm repo update'
5 participants