Fixes #178 #203
Fixes #178 #203
Conversation
Signed-off-by: João Vanzuita <joaovanzuita@me.com>
Signed-off-by: João Vanzuita <joaovanzuita@me.com>
Signed-off-by: João Vanzuita <joaovanzuita@me.com>
pkg/hub/repositories.go
Outdated
| @@ -77,12 +76,17 @@ func (c *Client) GetRepositories(account string) ([]Repository, int, error) { | |||
|
|
|||
| //RemoveRepository removes a repository on Hub | |||
| func (c *Client) RemoveRepository(repository string) error { | |||
| req, err := http.NewRequest("DELETE", c.domain+fmt.Sprintf(DeleteRepositoryURL, repository), nil) | |||
| repositoryUrl := fmt.Sprintf("%s%s%s/%s/", c.domain, RepositoriesURL, c.account, repository) | |||
There was a problem hiding this comment.
I'd like to point out that prefixing the user account to the repository name by default won't work. You can be part of many organizations, and so you may need to specify the full repository name <org>/<repo>.
With this code it will result with <user>/<org>/<repo>.
Maybe we should parse the repository argument, check if an org (or user) is already specified, and if not prefix with user account ? WDYT ?
There was a problem hiding this comment.
@silvin-lubecki tks for the review. That's true, I don't have access to a /paid account, and I couldn't test this use-case. I'll look for a paid account, and follow up with your comments.
There was a problem hiding this comment.
@silvin-lubecki I slept on that, and I believe the cleanest way to solve is to follow a pattern for the deletion. Allowing the user to not provide a prefix(username), and request the user to provide the organization name could be confusing.
My last commit makes it clear the user needs to always provide a prefix (username or organization name) together with the repository name. WDYT?
p.s: it was tested with user repo. and organization repo. Lint issue was also fixed, I wasn't aware of the linting 🙈
There was a problem hiding this comment.
Hello @converge, thank you for your PR 🙏
The new error handling is good to me, but not the account info part (see my other comment) 👍
|
By the way @converge the linter is complaining 🦁 |
Signed-off-by: João Vanzuita <joaovanzuita@me.com>
internal/commands/repo/rm.go
Outdated
| @@ -47,7 +47,7 @@ type rmOptions struct { | |||
| func newRmCmd(streams command.Streams, hubClient *hub.Client, parent string) *cobra.Command { | |||
| var opts rmOptions | |||
| cmd := &cobra.Command{ | |||
| Use: rmName + " [OPTIONS] REPOSITORY", | |||
| Use: rmName + " [OPTIONS] USERNAME(OR)ORGANIZATION NAME/REPOSITORY", | |||
There was a problem hiding this comment.
question: I wonder 🤔 if we should just rephrase to:
NAMESPACE/REPOSITORY
WDYT ?
There was a problem hiding this comment.
tks again for the guidance, it's great to have your support!
Signed-off-by: João Vanzuita <joaovanzuita@me.com>
- What I did
Fix one of the bugs reported by #178 and improve error handling for the repository deletion. The repository cannot be deleted if Docker Hub username is not provided.
how to reproduce the issue:
- How I did it
Error: resource not found- How to verify it
The impacted changes can be tested via:
./hub-tool repo rm repository-name- Description for the changelog
Allow repository deletion by using repository name only (without username) and add error handler for resource/repository not found.
p.s: this PR doesn't fix one of the reported errors:
Error: operation not permitted errorSince this is an ongoing discussion: #172
- A picture of a cute animal (not mandatory)