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: Added retention command #184

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Althaf66
Copy link
Collaborator

@Althaf66 Althaf66 commented Aug 25, 2024

Added new retention commands

harbor retention create - create retention policy for the project
harbor retention list - list retention execution for the project
harbor retention delete - delete retention policy for the project

this PR fixes part of #94

@Vad1mo Vad1mo requested a review from amands98 August 27, 2024 12:42
@bupd bupd self-requested a review November 30, 2024 00:12
@Jellyfrog
Copy link

Is there anything preventing this from being merged? (Except for the merge conflict)

Copy link
Member

@bupd bupd left a comment

Choose a reason for hiding this comment

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

Overall, Retention should be a subcommand of project or tag.

I believe it is better to have it as subcommand for tag. Since we also have tag immutability rules.

Example use case:

./harbor tag retention --projectname ...flags
./harbor tag immutable --projectname ...flags

The above would make much sense from a user perspective. Which is also clear to the user.

cmd := &cobra.Command{
Use: "retention",
Short: "Manage retention rule in the project",
Long: `Manage retention rules in the project in Harbor`,
Copy link
Member

Choose a reason for hiding this comment

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

We can use the Long explanation to explain what does this command do.
Since retention is a TAG RETENTION policy, we should communicate this well to the user.

Copy link
Member

Choose a reason for hiding this comment

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

Also mention, "user can only create 15 tag retention rules per project".

Use: "retention",
Short: "Manage retention rule in the project",
Long: `Manage retention rules in the project in Harbor`,
Example: `harbor retention create`,
Copy link
Member

Choose a reason for hiding this comment

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

some example usecases here would be beneficial.

func ListExecutionRetentionCommand() *cobra.Command {
cmd := &cobra.Command{
Use: "list",
Short: "list retention execution of the project",
Copy link
Member

Choose a reason for hiding this comment

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

This is an absolute wrong approach here.

As an user I would expect

./harbor project retention list

to return me the list of retention rules that I made. Instead of retention executions.

FIX THIS.

Comment on lines +19 to +20
Use: "list",
Short: "list retention execution of the project",
Copy link
Member

Choose a reason for hiding this comment

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

This is absolute wrong approach here.

As an user I would expect

./harbor tag retention list

to return me the list of retention rules that I made. Instead of retention executions.

FIX THIS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants