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

Add IAM samples #359

Merged
merged 4 commits into from
Apr 25, 2017
Merged

Add IAM samples #359

merged 4 commits into from
Apr 25, 2017

Conversation

ace-n
Copy link
Contributor

@ace-n ace-n commented Apr 25, 2017

@frankyn a few (nitpicky) questions for you:

  • Should we be calling our sample methods/region tags [view/add/remove]BucketIamRole instead of [view/add/remove]BucketIamMembers?
  • Do we need to explicitly specify project ID? The canonical Ruby IAM sample specifies it, but only some of the Node.js storage ones do. (Perhaps @jmdobry can chime in too?)

@ace-n ace-n requested a review from jmdobry April 25, 2017 00:16
storage/iam.js Outdated
// const roleName = "roles/storage.objectViewer";

// The list of IAM members to grant the role to
// const members = ['user:jdoe@example.com', 'group:admins@example.com'];
Copy link
Member

Choose a reason for hiding this comment

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

there's some extra indentation on this line

storage/iam.js Outdated
function viewBucketIamMembers (bucketName) {
// [START view_bucket_iam_members]

// Your Google Cloud Storage bucket name
Copy link
Member

Choose a reason for hiding this comment

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

Variable definitions (everything really) should be below any import statements.

storage/iam.js Outdated

function viewBucketIamMembers (bucketName) {
// [START view_bucket_iam_members]

Copy link
Member

Choose a reason for hiding this comment

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

there's an extra blank line here

storage/iam.js Outdated
function addBucketIamMember (bucketName, roleName, members) {
// [START add_bucket_iam_member]
// Your Google Cloud Storage bucket name
// const bucketName = "my-bucket";
Copy link
Member

Choose a reason for hiding this comment

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

Variable definitions (everything really) should be below any import statements

storage/iam.js Outdated
function removeBucketIamMember (bucketName, roleName, members) {
// [START remove_bucket_iam_member]
// Your Google Cloud Storage bucket name
// const bucketName = "my-bucket";
Copy link
Member

Choose a reason for hiding this comment

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

Variable definitions (everything really) should be below any import statements

storage/iam.js Outdated
.help();

if (module === require.main) {
cli.help().strict().argv; // eslint-disable-line
Copy link
Member

Choose a reason for hiding this comment

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

Maybe you're right, perhaps we should make this the standard (a "main" function).

Change to:

  .epilogue(`For more information, see https://cloud.google.com/iam/docs/overview and https://cloud.google.com/storage/docs`)
  .help()
  .strict();

if (module === require.main) {
  cli.parse(process.argv.slice(2));
}

@codecov-io
Copy link

codecov-io commented Apr 25, 2017

Codecov Report

Merging #359 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #359   +/-   ##
=======================================
  Coverage   83.88%   83.88%           
=======================================
  Files           4        4           
  Lines         422      422           
=======================================
  Hits          354      354           
  Misses         68       68

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 70fc057...1c94917. Read the comment docs.

@frankyn
Copy link
Member

frankyn commented Apr 25, 2017

Hi Ace,

To answer each of your questions:

Should we be calling our sample methods/region tags [view/add/remove]BucketIamRole instead of [view/add/remove]BucketIamMembers?

Keep BucketIamMembers as it makes more sense with the documentation that's being released. I'll send you additional information through chat.

Do we need to explicitly specify project ID? The canonical Ruby IAM sample specifies it, but only some of the Node.js storage ones do. (Perhaps @jmdobry can chime in too?)

I did it for Ruby because it's used more often. This falls into a per-language bias.

@ace-n ace-n merged commit 2109860 into master Apr 25, 2017
@ace-n ace-n deleted the bkt-iam branch April 25, 2017 21:48
grayside pushed a commit that referenced this pull request Oct 26, 2022
grayside pushed a commit that referenced this pull request Nov 3, 2022
ace-n pushed a commit that referenced this pull request Nov 14, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 14, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 14, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 15, 2022
Co-authored-by: release-please[bot] <55107282+release-please[bot]@users.noreply.github.com>
ace-n pushed a commit that referenced this pull request Nov 16, 2022
grayside pushed a commit that referenced this pull request Nov 16, 2022
ace-n pushed a commit that referenced this pull request Nov 17, 2022
ace-n pushed a commit that referenced this pull request Nov 17, 2022
grayside pushed a commit that referenced this pull request Nov 17, 2022
unforced pushed a commit that referenced this pull request Nov 17, 2022
* fix unmatched region tag

* fix lint

* fix bad copy paste

* fix another bad copy paste

* add new region tags to different demos

* update new tags to match most-used tag from other langs

* docs: remove unused region tags

Co-authored-by: Benjamin E. Coe <bencoe@google.com>
Co-authored-by: sofisl <55454395+sofisl@users.noreply.github.com>
ahrarmonsur pushed a commit that referenced this pull request Nov 17, 2022
ahrarmonsur pushed a commit that referenced this pull request Nov 17, 2022
Shabirmean pushed a commit that referenced this pull request Feb 16, 2023
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