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

projects.py: Do not error on delete if force is True #141

Closed
wants to merge 1 commit into from
Closed

projects.py: Do not error on delete if force is True #141

wants to merge 1 commit into from

Conversation

marcosps
Copy link
Collaborator

@marcosps marcosps commented Aug 4, 2023

No description provided.

Comment on lines +444 to +445
if response.status_code == 404 and force:
return True
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi there,

deletion by force is a feature of the OBS API:

Set to 1 if you want to delete the project even if the repositories of other projects include a path to this project.

Without knowing your use case I would argue, that the deletion of a non-existing project (forcefully or otherwise) warrants to return the objectified error message.

Speaking of error messages: If the status code of the response were 404, your if block would not get executed, because self.osc.request will raise a HTTPError.

Would you like to discuss options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's true... my use case is to just ignore if the project doesn't exist when force is specified but forgot about the exception...

I'm already handling the HTTPError in my project, but I was expecting of force argument to ignore non-existent projects... either way, we can drop this patch if you think the library user should take care of it :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for the description of your use case 🙂

Your use case makes sense to me, but I can also imagine that for other use cases, where OBS's force deletion is required, the exact error message might be relevant.

If it's OK with you, I'd suggest to handle the case of non-existing projects in your code instead of the library.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ok, makes sense. Thanks!

@marcosps marcosps closed this Aug 21, 2023
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.

2 participants