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

KMS: Makes samples consistent across languages. #459

Closed

Conversation

white0ut
Copy link

This change, in particular:

  • Consistently names variables with the API (locationId, keyRingId,
    cryptoKeyId).
  • Displays the function name in the region tag.
  • Correctly writes decoded base64 to disk and encodes file contents
    before calling KMS.
  • Labels parameters as plaintextFileName and ciphertextFileName, rather
    than inFile and outFile where applicable.

Tracking bug: http://b/64758639

This change, in particular:

* Consistently names variables with the API (locationId, keyRingId,
  cryptoKeyId).
* Displays the function name in the region tag.
* Correctly writes decoded base64 to disk  and encodes file contents
  before calling KMS.
* Labels parameters as plaintextFileName and ciphertextFileName, rather
  than inFile and outFile where applicable.

Tracking bug: http://b/64758639
@ace-n
Copy link
Contributor

ace-n commented Aug 22, 2017

I agree with most of these changes, save for the region tag modifications. These snippets are meant to be standalone pieces of code that can be run without being wrapped in a function - the sole purpose of the function wrapper is to make testing easier.

@jmdobry might have something to add though.

Copy link
Member

@jmdobry jmdobry left a comment

Choose a reason for hiding this comment

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

@ace-n is right. The variable name changes are fine, but the region tags need to go back to the way they were. Otherwise LGTM.

@codecov
Copy link

codecov bot commented Aug 23, 2017

Codecov Report

Merging #459 into master will decrease coverage by 70.46%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           master     #459       +/-   ##
===========================================
- Coverage   83.92%   13.46%   -70.47%     
===========================================
  Files           4        3        -1     
  Lines         423      208      -215     
===========================================
- Hits          355       28      -327     
- Misses         68      180      +112
Impacted Files Coverage Δ
vision/faceDetection.js 8.62% <0%> (-72.42%) ⬇️
vision/textDetection.js 12.05% <0%> (-62.42%) ⬇️
vision/quickstart.js 66.66% <0%> (-22.23%) ⬇️
vision/detect.js

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1df42ed...4f17304. Read the comment docs.

@white0ut
Copy link
Author

SGTM, thanks for the quick response.

Fixed.

@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that they're okay with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this State. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@jmdobry
Copy link
Member

jmdobry commented Aug 23, 2017

I moved your branch to #459 so the tests would run. Closing in favor of #459

@jmdobry jmdobry closed this Aug 23, 2017
NimJay pushed a commit that referenced this pull request Nov 10, 2022
- [x] Ensure the tests and linter pass
- [ obviously a decrease...] Code coverage does not decrease (if any source code was changed)
- [please review] Appropriate docs were updated (if necessary)

Samples are not in localized docs nor on C.G.C. These deletions are part of an internal 'clean-up' to reduce unused code in sample repositories.
NimJay pushed a commit that referenced this pull request Nov 10, 2022
- [x] Ensure the tests and linter pass
- [ obviously a decrease...] Code coverage does not decrease (if any source code was changed)
- [please review] Appropriate docs were updated (if necessary)

Samples are not in localized docs nor on C.G.C. These deletions are part of an internal 'clean-up' to reduce unused code in sample repositories.
ahrarmonsur pushed a commit that referenced this pull request Nov 18, 2022
* updated CHANGELOG.md [ci skip]

* updated package.json [ci skip]

* updated samples/package.json [ci skip]
ahrarmonsur pushed a commit that referenced this pull request Nov 18, 2022
* updated CHANGELOG.md [ci skip]

* updated package.json [ci skip]

* updated samples/package.json [ci skip]
NimJay pushed a commit that referenced this pull request Nov 18, 2022
* updated CHANGELOG.md [ci skip]

* updated package.json [ci skip]

* updated samples/package.json [ci skip]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants