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.

Sign up for GitHub

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

Jump to bottom

add projectID flag option to delete project command #324

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

add projectID flag option to delete project command #324

wants to merge 1 commit into from
+51 −20

Conversation

adityachopra29
Copy link

Which problem is this PR solving?

Description of the changes

  • add projectID flag to delete command in project command.
  • add projectID parameter to all corresponding functions
  • update project and repository handler functions accordingly

How was this change tested?

  • Running the delete command on one of the projects and verify it getting deleted:
    Screenshot 2025-02-07 at 4 54 55 AM

@adityachopra29
Copy link
Author

@bupd I have made the changes. Plz have a look if anything needs to be changed.
A few points I wanted to discuss:

  • In the client sdk for the operations on repositories, I found that all the querying methods only used projectName, unlike as provided in the api. Hence, I had to first use the updated getProject() method to first get the projectName from the projectID and then use it in the sdk methods. Is that the most optimal way or is there a better solution?
  • Methods defined in /prompt are not changed yet, as that would require making change in many more functions. If this is the correct way of solving this issue, I will make those changes as well.
  • A similiar issue as Force Project deletion does not work by id #309 is present for the rest of the repository operation commands (ie view, list) and project commands(such as project view ), as they also work only with projectName. Those changes can be made in this pr itself or later on, whatever is more suitable.

bupd
bupd suggested changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@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.

Thanks for you contribution

I have left a review on changes that can be made on this PR.

cmd/harbor/root/project/delete.go
@@ -45,6 +46,7 @@ func DeleteProjectCommand() *cobra.Command {

flags := cmd.Flags()
flags.BoolVar(&forceDelete, "force", false, "Deletes all repositories and artifacts within the project")
flags.BoolVar(&useProjectID, "projectID", false, "If set, treats the provided argument as a project ID instead of a project name")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest project ID to be a flag to which you can pass project ids. rather than having it as a boolean. since this would allow the args to always be projectname maintaining consistency.

Thanks

Copy link
Author

Choose a reason for hiding this comment

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

I would suggest project ID to be a flag to which you can pass project ids. rather than having it as a boolean. since this would allow the args to always be projectname maintaining consistency.

@bupd
So are you suggesting to make projectName an optional input for the delete command? Because I was under the understanding that we are giving projectID when we did not know the projectName.

Right now, when we don't give projectName, we enter the prompt mode and again choose projectName. So do we want to change that behavior when the projectID flag is given?

cmd/harbor/root/project/view.go
@@ -32,16 +32,16 @@ func ViewCommand() *cobra.Command {
Args: cobra.MaximumNArgs(1),
Run: func(cmd *cobra.Command, args []string) {
var err error
var projectName string
var projectNameOrID string
Copy link
Contributor

Choose a reason for hiding this comment

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

have it just projectName and have different var for projectID.

pkg/utils/helper.go
Comment on lines +156 to +160

func CreateBoolPointer(b bool) *bool {
return &b
}

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this

pkg/api/repository_handler.go
ctx, client, err := utils.ContextWithClient()
if err != nil {
return repository.ListRepositoriesOK{}, err
}
projectName := projectNameOrID
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong wording.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bupd bupd bupd requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Force Project deletion does not work by id
2 participants
@adityachopra29 @bupd