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: hmac service account #751

Merged
merged 61 commits into from
Aug 22, 2019
Merged

feat: hmac service account #751

merged 61 commits into from
Aug 22, 2019

Conversation

jkwlui
Copy link
Member

@jkwlui jkwlui commented Jun 20, 2019

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

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
Copy link
Contributor

@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
Contributor

bcoe commented Jun 24, 2019

looking fairly good to me, once tests are passing 😄

Copy link
Member

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

src/storage.ts Show resolved Hide resolved
src/storage.ts Show resolved Hide resolved
src/hmacKey.ts Outdated Show resolved Hide resolved
src/hmacKey.ts Outdated Show resolved Hide resolved
src/hmacKey.ts Outdated Show resolved Hide resolved
src/hmacKey.ts Outdated Show resolved Hide resolved
src/storage.ts Outdated Show resolved Hide resolved
src/storage.ts Outdated Show resolved Hide resolved
@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
* 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
Member

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
Member

frankyn commented Aug 8, 2019

LGTM, thanks @jkwlui!

@stephenplusplus
Copy link
Contributor

@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

* 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
* 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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants