Skip to content

feat: hmac service account #751

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
Merged
JustinBeckwith merged 61 commits into from
Aug 22, 2019
Merged

feat: hmac service account #751

JustinBeckwith merged 61 commits into from
Aug 22, 2019

Conversation

@jkwlui
Copy link
Member

@jkwlui jkwlui commented Jun 20, 2019 •
edited
Loading

Added the following Storage client methods:

  • createHmacKey
  • getHmacKeys
  • hmacKey

HmacKey class and methods:

  • get
  • delete
  • update

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 20, 2019
@codecov
Copy link

codecov bot commented Jun 20, 2019 •
edited
Loading

Codecov Report

Merging #751 into master will decrease coverage by 0.45%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #751      +/-   ##
==========================================
- Coverage   95.05%   94.59%   -0.46%     
==========================================
  Files          10       11       +1     
  Lines        1152     1202      +50     
  Branches      288      296       +8     
==========================================
+ Hits         1095     1137      +42     
- Misses         29       37       +8     
  Partials       28       28
Impacted Files Coverage Δ
src/hmacKey.ts 100% <100%> (ø)
src/index.ts 100% <100%> (ø) ⬆️
src/storage.ts 99.1% <100%> (+0.4%) ⬆️
bin/benchwrapper.js 0% <0%> (ø) ⬆️

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 48f9b44...1b80e24. Read the comment docs.

@jkwlui jkwlui added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 21, 2019
@jkwlui jkwlui requested review from a team and frankyn June 21, 2019 21:27
@jkwlui jkwlui self-assigned this Jun 21, 2019
bcoe
bcoe approved these changes Jun 24, 2019
View reviewed changes
Copy link

@bcoe bcoe left a comment

Choose a reason for hiding this comment

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

this is looking pretty lovely to me, on a quick read.

@bcoe
Copy link

bcoe commented Jun 24, 2019

looking fairly good to me, once tests are passing 😄

frankyn
frankyn suggested changes Jun 26, 2019
View reviewed changes
Copy link
Contributor

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

Overall, looks good. I have two nit questions.

stephenplusplus
stephenplusplus reviewed Jun 26, 2019
View reviewed changes
stephenplusplus
stephenplusplus reviewed Jun 26, 2019
View reviewed changes
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Jun 27, 2019
@kokoro-team kokoro-team removed the kokoro:run Add this label to force Kokoro to re-run the tests. label Jul 2, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Jul 3, 2019
@jkwlui jkwlui added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jul 3, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 5, 2019
frankyn
frankyn reviewed Aug 6, 2019
View reviewed changes
src/storage.ts
* Note: this does not fetch the HMAC key's metadata. Use HmacKey#get() to
* retrieve and populate the metadata.
*
* @param {string} accessId The HMAC key's access ID.
Copy link
Contributor

@frankyn frankyn Aug 6, 2019

Choose a reason for hiding this comment

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

@jkwlui could you add an optional parameter for projectId? This would help in the case the user doesn't want to use the project defined by credentials or GOOGLE_CLOUD_PROJECT.

@frankyn
Copy link
Contributor

frankyn commented Aug 8, 2019

LGTM, thanks @jkwlui!

@stephenplusplus
Copy link
Contributor

stephenplusplus commented Aug 9, 2019

@jkwlui a new linty error has appeared, seemingly after I updated this PR to use the new common:

src/hmacKey.ts:323:7 - error TS2322: Type '{ delete: boolean; get: boolean; getMetadata: boolean; setMetadata: { reqOpts: { method: string; }; }; }' is not assignable to type 'Methods'.
  Property 'setMetadata' is incompatible with index signature.
    Type '{ reqOpts: { method: string; }; }' is not assignable to type 'boolean | { reqOpts?: CoreOptions | undefined; }'.
      Type '{ reqOpts: { method: string; }; }' is not assignable to type '{ reqOpts?: CoreOptions | undefined; }'.
        Types of property 'reqOpts' are incompatible.
          Type '{ method: string; }' is not assignable to type 'CoreOptions'.
            Types of property 'method' are incompatible.
              Type 'string' is not assignable to type '"GET" | "HEAD" | "POST" | "DELETE" | "PUT" | "CONNECT" | "OPTIONS" | "TRACE" | "PATCH" | undefined'.
323       methods,
          ~~~~~~~
  node_modules/@google-cloud/common/build/src/service-object.d.ts:54:5
    54     methods?: Methods;
           ~~~~~~~
    The expected type comes from property 'methods' which is declared here on type 'ServiceObjectConfig'

tenor

jkwlui added 3 commits August 9, 2019 13:37
@jkwlui
fix(ts): HmacKey Methods
ea82660
@jkwlui
Merge branch 'master' into hmac-sa-admin
b270054
@jkwlui
feat: HMAC System tests (#750)
a4c4a66 
* npm run fix

* test: add second service account email for testing multiple SAs HMAC keys

* npm run fix

* test: add second service account email for testing multiple SAs HMAC keys

* fix tests

* use HMAC_PROJECT in env variable
@jkwlui jkwlui changed the title feat: HMAC Service Account CRUD methods feat: hmac service account Aug 14, 2019
jkwlui added 8 commits August 14, 2019 15:29
@jkwlui
feat(test): hmac key - service account pool and fix tests (#810)
28f5f33 
* update HMAC samples to not require creds, and add projectId

* setup second system test service account

* fix sample tests

* fix sample metadata for hmac keys

* reorder hmackey argument

* run synthtool

* put projectId at the end so its optional

* synthtool

* fix cleanup after test

* log service account used

* fixy

* fix(tests): create needs projectId

* npm run fix

* use gimmeproj instead of gimme-acc

* fix hmacKeyGet

* fix system-test

* fix check active

* fix HMAC_PROJECT in system-test

* properly unlease all service accounts

* fix unleasing
@jkwlui
Merge branch 'master' into hmac-sa-admin
b1a2c61
@jkwlui
Merge branch 'master' into hmac-sa-admin
25ac621
@jkwlui
npm run fix
05e37be
@jkwlui
Merge branch 'master' into hmac-sa-admin
c816660
@jkwlui
fix copyright header
19be674
@jkwlui
Merge branch 'hmac-sa-admin' of github.com:googleapis/nodejs-storage …
bd26755 
…into hmac-sa-admin
@jkwlui
Merge branch 'master' into hmac-sa-admin
48b7c43
@jkwlui jkwlui added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 20, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Aug 20, 2019
@jkwlui
Copy link
Member Author

jkwlui commented Aug 21, 2019

The tests are blocked by #818

@JustinBeckwith JustinBeckwith merged commit ed1ec7b into master Aug 22, 2019
@stephenplusplus stephenplusplus deleted the hmac-sa-admin branch August 22, 2019 19:42
@jkwlui jkwlui mentioned this pull request Sep 3, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@frankyn frankyn frankyn approved these changes

@bcoe bcoe bcoe approved these changes

@stephenplusplus stephenplusplus stephenplusplus approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jkwlui jkwlui

Labels

cla: yes This human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@jkwlui @bcoe @frankyn @stephenplusplus @JustinBeckwith @googlebot @kokoro-team @yoshi-automation