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

Spanner: add 'operation_id' parameter 'Database.update_ddl' (#6737) #6825

Merged

Conversation

potiuk
Copy link
Contributor

@potiuk potiuk commented Dec 2, 2018

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.

@potiuk potiuk requested a review from crwilcox as a code owner December 2, 2018 20:30
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 2, 2018
@tseaver tseaver changed the title Added missing operation_id to update_ddl method (#6737) Spanner: add 'operation_id' parameter 'Database.update_ddl' (#6737) Dec 3, 2018
@tseaver tseaver added the api: spanner Issues related to the Spanner API. label Dec 3, 2018
Copy link
Contributor

@tseaver tseaver left a 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 copy test_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.

spanner/google/cloud/spanner_v1/database.py Outdated Show resolved Hide resolved
@potiuk potiuk force-pushed the ADD_OPERATION_ID_IN_UPDATE_DDL branch from 9c2d2bd to bc526b0 Compare December 4, 2018 11:45
@potiuk
Copy link
Contributor Author

potiuk commented Dec 4, 2018

@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).

@tseaver
Copy link
Contributor

tseaver commented Dec 4, 2018

@potiuk OK, rather than adding another skipped system test, why don't we just pass the generated operation ID in the existing skipped test?

@potiuk potiuk force-pushed the ADD_OPERATION_ID_IN_UPDATE_DDL branch from bc526b0 to e9745de Compare December 4, 2018 20:27
@potiuk
Copy link
Contributor Author

potiuk commented Dec 4, 2018

@tseaver Sure. Removed the old tests.

@tseaver tseaver merged commit f9a725c into googleapis:master Dec 4, 2018
@tseaver
Copy link
Contributor

tseaver commented Dec 4, 2018

@potiuk Thank you again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: spanner Issues related to the Spanner API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants