Skip to content

IAM: Add access code snippets and tests #1597

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
kurtisvg merged 9 commits into from
Oct 3, 2019
Merged

IAM: Add access code snippets and tests #1597

kurtisvg merged 9 commits into from
Oct 3, 2019

Conversation

@melaniedejong
Copy link
Contributor

@melaniedejong melaniedejong commented Oct 2, 2019

Added access code snippets and tests for these docs:
https://cloud.google.com/iam/docs/granting-changing-revoking-access
https://cloud.google.com/iam/docs/testing-permissions
(I don't know how to not include previously merged commits, so please forgive the slight clutter)

@melaniedejong melaniedejong requested a review from a team October 2, 2019 00:04
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 2, 2019
lesv
lesv reviewed Oct 2, 2019
View reviewed changes
lesv
lesv suggested changes Oct 2, 2019
View reviewed changes
Copy link
Contributor

@lesv lesv left a comment

Choose a reason for hiding this comment

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

PTAL
It's idiomatic to throw errors, just annotate your methods so its known, unless you are actually doing something.

Regarding submitting too much. I use a tool like Github Desktop and my flow is:

  1. Sync to head on master
  2. create a new branch and switch to it.
  3. write modify code
  4. commit
  5. push to github
  6. check
  7. create a PR
  8. modify for feedback
  9. commit
  10. push

Once there is an approval:

  1. switch to master and sync
  2. delete the prior branch
  3. create a new branch for further work.

iam/api-client/src/main/java/com/google/iam/snippets/TestPermissions.java
Comment on lines +41 to +44
} catch (IOException | GeneralSecurityException e) {
System.out.println("Unable to initialize service: \n" + e.toString());
return;
}
Copy link
Contributor

@lesv lesv Oct 2, 2019

Choose a reason for hiding this comment

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

not idiomatic

@melaniedejong melaniedejong requested a review from lesv October 2, 2019 19:24
@melaniedejong
Copy link
Contributor Author

melaniedejong commented Oct 3, 2019

@lesv Any idea why the checks are still pending?

@lesv lesv added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 3, 2019
@lesv
Copy link
Contributor

lesv commented Oct 3, 2019

@dpebot merge when green
@melaniedejong I've tried forcing Kokoro to run.

@dpebot
Copy link
Contributor

dpebot commented Oct 3, 2019

Okay! I'll merge when all statuses are green and all reviewers approve.

@dpebot dpebot added the automerge Merge the pull request once unit tests and other checks pass. label Oct 3, 2019
@dpebot dpebot assigned dpebot and unassigned lesv Oct 3, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 3, 2019
@lesv lesv requested review from kurtisvg and removed request for kurtisvg October 3, 2019 19:54
@lesv
Copy link
Contributor

lesv commented Oct 3, 2019

@kurtisvg I was trying to remove you from the reviewer list and accidentally requested your review.

@melaniedejong
Copy link
Contributor Author

melaniedejong commented Oct 3, 2019

I don't know how it's failing if the only thing that's been changed was merging in the master branch

@kurtisvg
Copy link
Contributor

kurtisvg commented Oct 3, 2019 

------------------------------------------------------------
- testing iam/api-client
------------------------------------------------------------
[ERROR] src/main/java/com/google/iam/snippets/TestPermissions.java:[35] (indentation) Indentation: 'method def modifier' has incorrect indentation level 0, expected level should be 2.
[ERROR] src/main/java/com/google/iam/snippets/TestPermissions.java:[46] (sizes) LineLength: Line is longer than 100 characters (found 116).
[ERROR] src/main/java/com/google/iam/snippets/TestPermissions.java:[55] (sizes) LineLength: Line is longer than 100 characters (found 101).
[ERROR] src/main/java/com/google/iam/snippets/TestPermissions.java:[55,101] (whitespace) OperatorWrap: '+' should be on a new line.
[ERROR] src/main/java/com/google/iam/snippets/RemoveMember.java:[36,9] (whitespace) WhitespaceAround: WhitespaceAround: 'if' is not followed by whitespace. Empty blocks may only be represented as {} when not part of a multi-block statement (4.1.3)
[ERROR] src/main/java/com/google/iam/snippets/RemoveMember.java:[36,44] (whitespace) WhitespaceAround: WhitespaceAround: '{' is not preceded with whitespace.
[ERROR] src/main/java/com/google/iam/snippets/RemoveMember.java:[37] (indentation) Indentation: 'if' child has incorrect indentation level 12, expected level should be 10.
[ERROR] src/main/java/com/google/iam/snippets/RemoveMember.java:[38] (indentation) Indentation: 'if' child has incorrect indentation level 12, expected level should be 10.
[ERROR] src/main/java/com/google/iam/snippets/RemoveMember.java:[40,40] (whitespace) WhitespaceAround: WhitespaceAround: '{' is not preceded with whitespace.
[ERROR] src/main/java/com/google/iam/snippets/RemoveMember.java:[41] (indentation) Indentation: 'if' child has incorrect indentation level 12, expected level should be 10.
[ERROR] src/test/java/com/google/iam/snippets/AccessTests.java:[59] (coding) VariableDeclarationUsageDistance: Distance between variable 'bindings' declaration and its first usage is 4, but allowed 3.  Consider making that variable final if you still need to store its value in advance (before method calls that might have side effects on the original value).
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check (default) on project iam-snippets: You have 11 Checkstyle violations. -> [Help 1]

@melaniedejong
Copy link
Contributor Author

melaniedejong commented Oct 3, 2019

Ah, thank you!

@kurtisvg kurtisvg added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 3, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Oct 3, 2019
@kurtisvg kurtisvg merged commit a0be08f into GoogleCloudPlatform:master Oct 3, 2019
@melaniedejong melaniedejong deleted the testing-permissions branch October 4, 2019 18:15
@melaniedejong melaniedejong mentioned this pull request Sep 17, 2020
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@kurtisvg kurtisvg kurtisvg left review comments

@lesv lesv lesv approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dpebot dpebot

Labels

automerge Merge the pull request once unit tests and other checks pass. 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.

6 participants

@melaniedejong @lesv @dpebot @kurtisvg @googlebot @kokoro-team