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.

Sign up for GitHub

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

Jump to bottom

fix: Add type hints for KMS snippets #9979

Merged
merged 13 commits into from May 19, 2023
Merged

fix: Add type hints for KMS snippets #9979

merged 13 commits into from May 19, 2023

Conversation

glasnt
Copy link
Collaborator

@glasnt glasnt commented May 17, 2023 •
edited by rsamborski

Description

Fixes b/280879322

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

@product-auto-label product-auto-label bot added samples Issues that are directly related to samples. api: cloudkms Issues related to the Cloud Key Management Service API. labels May 17, 2023
@glasnt glasnt marked this pull request as ready for review May 17, 2023 06:11
@glasnt glasnt requested review from a team as code owners May 17, 2023 06:11
rsamborski
rsamborski requested changes May 17, 2023
View reviewed changes
Copy link
Member

@rsamborski rsamborski left a comment

Choose a reason for hiding this comment

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

Great work. Thank you.

I have one request which applies to all samples that require a new import outside of the sample function:

  1. Please make sure the region tag includes the imports and avoids duplication (i.e. importing kms again inside the function). Otherwise if someone c&p the sample from c.g.c. it will fail without the import.
  2. Optionally as this is more cumbersome, we could move out all imports from sample function and keep them together in the top of the file.

kms/snippets/create_key_asymmetric_sign.py Show resolved Hide resolved
kms/snippets/create_key_asymmetric_decrypt.py Show resolved Hide resolved
kms/snippets/create_key_asymmetric_decrypt.py Outdated Show resolved Hide resolved
kms/snippets/create_key_asymmetric_decrypt.py Outdated Show resolved Hide resolved
kms/snippets/create_key_asymmetric_sign.py Outdated Show resolved Hide resolved
kms/snippets/create_key_asymmetric_sign.py Outdated Show resolved Hide resolved
kms/snippets/create_key_hsm.py Show resolved Hide resolved
kweinmeister
kweinmeister approved these changes May 17, 2023
View reviewed changes
Copy link
Contributor

@kweinmeister kweinmeister left a comment

Choose a reason for hiding this comment

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

Approving, assuming @rsamborski 's comments are addressed

@glasnt
Copy link
Collaborator Author

glasnt commented May 19, 2023

I was using isort --profile google while testing this, and found a bug: PyCQA/isort#2136, but nox uses flake8 with flake8-import-order, so this error isn't coming up in CI.

rsamborski
rsamborski approved these changes May 19, 2023
View reviewed changes
Copy link
Member

@rsamborski rsamborski left a comment

Choose a reason for hiding this comment

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

Thanks. Approved.

@rsamborski rsamborski merged commit b003718 into main May 19, 2023
11 checks passed
@rsamborski rsamborski deleted the fixit-glasnt-typehints-kms branch May 19, 2023 12:24
m-strzelczyk pushed a commit that referenced this pull request May 19, 2023
@glasnt @m-strzelczyk
fix: Add type hints for KMS snippets (#9979)
85f170b 
* fix: Add type hints for KMS snippets

* fix: type

* fix: even more types

* black

* fix: remove debugging note

* isort

* manual sort, apparently

* Move all imports to top of files, sort, move region tags

* even more import fixes

* black, isort

* black, again
m-strzelczyk added a commit that referenced this pull request May 27, 2023
@m-strzelczyk @engelke
@renovate-bot @kubasieron @Avani-Thakker-Crest @averikitsch @glasnt @kweinmeister
fixit: Add type-hints to Python sample at functions/ocr/app (#9990)
8b4947d 
* fixit: Add type-hints to Python sample at functions/ocr/app

* Fixing stuff

* Fixing stuff

* Enforcing type checks

* Enforcing type checks

* Fixing any typing

* chore(deps): update dependency shapely to v2 (#10004)

* Adding deletion of rows with NULL dag_id (#10002)

Co-authored-by: Charles Engelke <engelke@google.com>

* fixit: add type-hints to functions/v2/ocr (#9980)

b/280879671

## Description

Fixes b/280879671

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

## Checklist
- [ ] I have followed [Sample Guidelines from AUTHORING_GUIDE.MD](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md)
- [ ] README is updated to include [all relevant information](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md#readme-file)
- [ ] **Tests** pass:   `nox -s py-3.9` (see [Test Environment Setup](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md#test-environment-setup))
- [ ] **Lint** pass:   `nox -s lint` (see [Test Environment Setup](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md#test-environment-setup))
- [ ] These samples need a new **API enabled** in testing projects to pass (let us know which ones)
- [ ] These samples need a new/updated **env vars** in testing projects set to pass (let us know which ones)
- [ ] This sample adds a new sample directory, and I updated the [CODEOWNERS file](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/.github/CODEOWNERS) with the codeowners for this sample
- [ ] This sample adds a new **Product API**, and I updated the [Blunderbuss issue/PR auto-assigner](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/.github/blunderbuss.yml) with the codeowners for this sample
- [ ] Please **merge** this PR for me once it is approved

* [DLP] Implemenetd dlp_inspect_image_listed_infotypes with unit test cases (#9872)

## Description
Implemenetd dlp_inspect_image_listed_infotypes with unit test cases. Java equivalent: https://cloud.google.com/dlp/docs/samples/dlp-inspect-image-listed-infotypes#dlp_inspect_image_listed_infotypes-java

Fixes #<ISSUE-NUMBER>

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

## Checklist
- [X] I have followed [Sample Guidelines from AUTHORING_GUIDE.MD](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md)
- [ ] README is updated to include [all relevant information](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md#readme-file)
- [X] **Tests** pass:   `nox -s py-3.9` (see [Test Environment Setup](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md#test-environment-setup))
- [X] **Lint** pass:   `nox -s lint` (see [Test Environment Setup](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/AUTHORING_GUIDE.md#test-environment-setup))
- [ ] These samples need a new **API enabled** in testing projects to pass (let us know which ones)
- [ ] These samples need a new/updated **env vars** in testing projects set to pass (let us know which ones)
- [ ] This sample adds a new sample directory, and I updated the [CODEOWNERS file](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/.github/CODEOWNERS) with the codeowners for this sample
- [ ] This sample adds a new **Product API**, and I updated the [Blunderbuss issue/PR auto-assigner](https://togithub.com/GoogleCloudPlatform/python-docs-samples/blob/main/.github/blunderbuss.yml) with the codeowners for this sample
- [X] Please **merge** this PR for me once it is approved

* chore(deps): update dependency structlog to v22.3.0 (#8540)

Co-authored-by: Charles Engelke <engelke@google.com>

* fixit: update docstrings for Cloud Run samples (#10007)

* chore: update docstrings for Cloud Run samples

* lint

* Update render.py

* fix: Add type hints for KMS snippets (#9979)

* fix: Add type hints for KMS snippets

* fix: type

* fix: even more types

* black

* fix: remove debugging note

* isort

* manual sort, apparently

* Move all imports to top of files, sort, move region tags

* even more import fixes

* black, isort

* black, again

* Fixing typing info

* Trying to fix typing

---------

Co-authored-by: Charles Engelke <engelke@google.com>
Co-authored-by: Mend Renovate <bot@renovateapp.com>
Co-authored-by: kubasieron <89135874+kubasieron@users.noreply.github.com>
Co-authored-by: Avani-Thakker-Crest <129363704+Avani-Thakker-Crest@users.noreply.github.com>
Co-authored-by: Averi Kitsch <akitsch@google.com>
Co-authored-by: Katie McLaughlin <katie@glasnt.com>
Co-authored-by: Karl Weinmeister <11586922+kweinmeister@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rsamborski rsamborski rsamborski approved these changes

@kweinmeister kweinmeister kweinmeister approved these changes

Assignees

@msampathkumar msampathkumar

Labels
api: cloudkms Issues related to the Cloud Key Management Service API. samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@glasnt @rsamborski @kweinmeister @msampathkumar