-
Notifications
You must be signed in to change notification settings - Fork 10
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: project delete #112
feat: project delete #112
Conversation
This pull request has been linked to Shortcut Story #18973: CLI vNext: project [create|delete|list|view]. |
update go-octopusdeploy reference
This pull request has been linked to Shortcut Story #26927: CLI vNext: project delete. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just the remove unused thing changes @borland has requested and some questions but happy to move the question changes to a future PR as they depend on changes we have made with project create
.
Overall, good stuff!
"io" | ||
) | ||
|
||
type DeleteOptions struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's update this to use the new dependencies struct we made for project create, maybe in a future PR?
For now let's remove the opts that are not being used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 I have removed the unused ones, can fix things up after project create
goes in
pkg/cmd/project/delete/delete.go
Outdated
itemToDelete, err := opts.Client.Projects.GetByIdOrName(opts.idOrName) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if opts.DeleteFlags.Confirm.Value { | ||
return delete(opts.Client, itemToDelete) | ||
} else { | ||
return question.DeleteWithConfirmation(opts.Ask, "project", itemToDelete.Name, itemToDelete.ID, func() error { | ||
return delete(opts.Client, itemToDelete) | ||
}) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking if we should move this to a Commit
function to keep consistent with the new pattern?
Not sure if we should as I don't believe delete commands should be dependable
, thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be no reason for delete to have dependencies or be a dependency. Cascade deletes should be handled by the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
[sc-26927]
delete via name
delete via slug
delete with confirm