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

change behavior when add repo with same name and url. #8772

Closed
wants to merge 1 commit into from
Closed

change behavior when add repo with same name and url. #8772

wants to merge 1 commit into from

Conversation

yxxhero
Copy link
Member

@yxxhero yxxhero commented Sep 20, 2020

Signed-off-by: yxxhero aiopsclub@163.com

What this PR does / why we need it:
resolve: #8771
We could just return exit code zero if the same repo is attempted to be added twice with the same name.

Special notes for your reviewer:

If applicable:

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

Signed-off-by: yxxhero <aiopsclub@163.com>
Copy link

@joaopaulosr95 joaopaulosr95 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@bacongobbler bacongobbler left a comment

Choose a reason for hiding this comment

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

Thank you for the quick PR. However while reviewing, I do not believe there is need for some of the function calls added here. These newly introduced function calls are overly verbose and are leaky in design.

I will submit an alternative PR to fix this issue. Thank you for your effort.

@@ -85,6 +85,22 @@ func (r *File) Has(name string) bool {
return entry != nil
}

// HasRepoWithNameAndURL returns true if the given name whth url is already a repository name.
func (r *File) HasRepoWithNameAndURL(name string, url string) bool {
Copy link
Member

Choose a reason for hiding this comment

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

Why are we introducing these functions?

}

// GetRepoByNameAndURL returns an entry with the given name and url if it exists, otherwise returns nil
func (r *File) GetRepoByNameAndURL(name string, url string) *Entry {
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary?

@bacongobbler
Copy link
Member

See #8777.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

helm repo add now fails in v3.3.2 and v3.3.3
4 participants