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

feat: allow multiple KMS keys to create CMEK database/backup #1191

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

hwin16
Copy link

@hwin16 hwin16 commented Aug 28, 2024

Add kms_key_names field to create database/create backup code snippets.

@hwin16 hwin16 requested review from a team as code owners August 28, 2024 00:44
@hwin16 hwin16 requested a review from nicain August 28, 2024 00:44
Copy link

snippet-bot bot commented Aug 28, 2024

Here is the summary of changes.

You are about to add 4 region tags.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: spanner Issues related to the googleapis/python-spanner API. samples Issues that are directly related to samples. labels Aug 28, 2024
@hwin16
Copy link
Author

hwin16 commented Aug 28, 2024

I am a googler following self-serve guide.

@harshachinta
Copy link
Contributor

@hwin16 The handwritten changes are not needed since it is a admin feature.
The samples look good. Will approve the PR once the handwritten changes are removed.

@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 30, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 30, 2024
@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 5, 2024
Copy link

conventional-commit-lint-gcf bot commented Sep 5, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@product-auto-label product-auto-label bot added size: m Pull request size is medium. and removed size: l Pull request size is large. labels Sep 5, 2024
@hwin16
Copy link
Author

hwin16 commented Sep 5, 2024

Thank you @harshachinta! PTAL.

@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 5, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 5, 2024
@harshachinta
Copy link
Contributor

harshachinta commented Sep 5, 2024

@hwin16 Can you please fix the lint by running nox -s blacken in samples/samples folder.
Also sample tests are failing. please fix

@product-auto-label product-auto-label bot added size: l Pull request size is large. and removed size: m Pull request size is medium. labels Sep 5, 2024
@hwin16
Copy link
Author

hwin16 commented Sep 5, 2024

@harshachinta , I have fixed the linting issue. I fixed some issues with integration tests but wasn't able to run them. Our feature is adding repeated field to proto and similar to existing unit test "create_database_with_encryption_key". When I run existing unit test, that is failing without any changes. Looks like instance set up is incorrect in integration test?

Screenshot 2024-09-05 at 2 22 20 PM

@harshachinta
Copy link
Contributor

@hwin16
This test is dependent on another test that creates the instance once. The dependency annotation is missing for these tests. Let me fix it in seperate PR.
However you can run the following command to get it working,

nox -s py-3.11 -- -k "test_create_database_explicit or test_create_database_with_encryption_config"

@hwin16
Copy link
Author

hwin16 commented Sep 11, 2024

@harshachinta , done fixing integration tests and they are passing now. Currently, skipped them before feature becomes public.

Passing test results:
Screenshot 2024-09-11 at 1 52 52 PM

@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2024
@harshachinta harshachinta added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2024
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2024
@pytest.mark.dependency(name="create_backup_with_multiple_kms_keys")
def test_create_backup_with_multiple_kms_keys(
capsys,
multi_region_instance,
Copy link
Contributor

@harshachinta harshachinta Sep 12, 2024

Choose a reason for hiding this comment

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

Do we have a fixture for this multi_region_instance?
Where is this instance getting create? I am not able to find in this code

@harshachinta
Copy link
Contributor

LGTM with couple of questions. Once the backend is ready we can merge this PR.

@harshachinta harshachinta added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Sep 12, 2024
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 googleapis/python-spanner API. do not merge Indicates a pull request not ready for merge, due to either quality or timing. samples Issues that are directly related to samples. size: l Pull request size is large.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants