-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
integration: test_create_volume_invalid_driver allow either 400 or 404 #3296
integration: test_create_volume_invalid_driver allow either 400 or 404 #3296
Conversation
cd75d97
to
246207f
Compare
tests/integration/api_volume_test.py
Outdated
self.client.create_volume('perfectcherryblossom', driver_name) | ||
assert ( |
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.
Shouldn't this assert be in the with
block?
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.
oh! quite likely. It's been a while since I wrote this patch, so don't recall what I did here 😂
Let me fix!
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.
Updated! ❤️
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.
At least, I think that's what you meant, right? 🙈
(don't trust me with your Python code)
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.
Yup, I am pretty sure it would work either way. The with:
block should clean up anything it created (the 'as' variables) when its scope ends, but IIRC the variable names stay around (they only get removed when a proper scope (function) ends). I've not done much python in 7 (or more 🤔) years so I may be misremembering the details.
The API currently returns a 404 error when trying to create a volume with an invalid (non-existing) driver. We are considering changing this status code to be a 400 (invalid parameter), as even though the _reason_ of the error may be that the plugin / driver is not found, the _cause_ of the error is that the user provided a plugin / driver that's invalid for the engine they're connected to. This patch updates the test to pass for either case. Signed-off-by: Sebastiaan van Stijn <[email protected]>
246207f
to
fad84c3
Compare
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
The API currently returns a 404 error when trying to create a volume with an invalid (non-existing) driver. We are considering changing this status code to be a 400 (invalid parameter), as even though the reason of the error may be that the plugin / driver is not found, the cause of the error is that the user provided a plugin / driver that's invalid for the engine they're connected to.
This patch updates the test to pass for either case.