feat: Add DEL endpoint for deployments and related tests#18
Conversation
mtruj013
force-pushed
the
delete-deployment-endpoint
branch
from
5cc156d to
e234830
Compare
| "reason": ("Products must have at least one deployment.") | ||
| }, | ||
| } | ||
| }, 400 |
There was a problem hiding this comment.
| }, 400 | |
| }, 409 |
I believe a 409 better suits this case
There was a problem hiding this comment.
409 is supposed to be resolvable if the client retries the request (eg server is receiving multiple requests to update the same resource). But this isn't resolvable because the user will never be able to delete the last deployment regardless of how they pass the request
| try: | ||
| db.session.commit() | ||
| except Exception: | ||
| db.session.remove() |
There was a problem hiding this comment.
| db.session.remove() | |
| db.session.rollback() |
Since the intent here is to discard the pending transaction after a failed commit()
There was a problem hiding this comment.
The intent is correct, but this change would do the opposite. From sqlalchemy docs: "(scoped.session.remove)... releases any existing transactional/connection resources still being held; transactions specifically are rolled back."
| } | ||
| }, 500 | ||
|
|
||
| return {"deleted": deleted}, 200 |
There was a problem hiding this comment.
is this object needed?
Because for a successful deletion which is 204 No Content we should return "", 204 and remove the need for the deleted = {...} snapshot.
If we want the deleted representation instead, return DeploymentSchema().dump(deployment) (snapshotted before session.delete)
There was a problem hiding this comment.
The users of auth protected endpoints (auth implementation blocked on prodstack status if wondering) will include product owners, the clear outputs using 200 are an intentional UX decision because of the intended audience (Stripe does something similar). The snapshot would include way more info than is actually needed so I'm not sure that it'd be much of an improvement.
More than happy to come back to this if after stakeholder feedback on this more complete version comes back that this isn't super needed, but for now I'd opt to keep as is.
mtruj013
force-pushed
the
delete-deployment-endpoint
branch
from
e234830 to
ee53667
Compare
Onibenjo
added
Review: Code +1
Review: QA +1
and removed
Review: Code needed
Review: QA needed
labels
2 participants
Done
QA
Issue / Card
https://warthogs.atlassian.net/browse/WD-35418