-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Spanner: add 'operation_id' parameter 'Database.update_ddl' (#6737) #6825
Spanner: add 'operation_id' parameter 'Database.update_ddl' (#6737) #6825
Conversation
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.
Thanks very much for the patch!
Rather than adding another always-skipped system test, could you please add a unit test for the feature in tests/unit/test_database.py
?
- Copy
test_update_ddl
, naming the copytest_update_ddl_w_operation_id
. - In the copy, invoke the method with the
operation_id
argument. - Update the assertion at the end to match the passed argument.
9c2d2bd
to
bc526b0
Compare
@tseaver: I added the unit test (I have not noticed that those exist for update_ddl in the first place). But I also left the skipped test there, as I found it really useful to test it with real instance. It simply makes it easier - especially that I tried to generate the id based on docs of Spanner and it turned out that the id cannot be too long. Having an example working ID generation in test might be a good hint for someone trying to do the same (if they start to look at the source code). |
@potiuk OK, rather than adding another skipped system test, why don't we just pass the generated operation ID in the existing skipped test? |
bc526b0
to
e9745de
Compare
@tseaver Sure. Removed the old tests. |
@potiuk Thank you again! |
The operation_id im update_ddl method is missing. The parameter is in the documentation https://googleapis.github.io/google-cloud-python/latest/spanner/database-usage.html#update-an-existing-database but it was missing in the implementation.
I've run system tests manually - even if the update_ddl system tests are skipped due to flakyness (I removed the skip annotations and increased timeouts) so I confirm it works :).
Closes #6737.