feat: add IAM Conditions support #120

frankyn · 2020-02-06T18:38:44Z

Add IAM Conditions Support.

Depends on:
googleapis/java-core#110

Pending Review List:

@jkwlui (IAM Conditions Review)
@JesseLovelace (Java + Storage Review)

jkwlui

Surface looks good. LGTM!
I'll let @chingor13 and others review Java aspects.

elharo

Mostly nits, but snapshot dependencies are a hard no.

elharo · 2020-02-11T16:14:52Z

google-cloud-storage/src/main/java/com/google/cloud/storage/PolicyHelper.java

+      Bindings apiBinding = new Bindings();
+      apiBinding.setRole(binding.getRole());
+      apiBinding.setMembers(new ArrayList<>(binding.getMembers()));
+      if (null != binding.getCondition()) {


no reason to put null first; it's a leftover convention from C that doesn't make sense in Java

elharo · 2020-02-11T16:15:54Z

google-cloud-storage/src/test/java/com/google/cloud/storage/PolicyHelperTest.java

-
-    assertEquals(libPolicy, actualLibPolicy);
-    assertTrue(new ApiPolicyMatcher(apiPolicy).matches(actualApiPolicy));
+//    Policy actualLibPolicy = PolicyHelper.convertFromApiPolicy(apiPolicy);


delete it if it isn't relevant. or fix it if it is.

elharo · 2020-02-11T16:16:31Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

@@ -2329,6 +2331,8 @@ public void testReadCompressedBlob() throws IOException {
  public void testBucketPolicy() {
    testBucketPolicyRequesterPays(true);
    testBucketPolicyRequesterPays(false);
+    // testBucketPolicyV3RequesterPays(true);


don't comment out code

elharo · 2020-02-11T16:17:14Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

@@ -2398,6 +2402,160 @@ private void testBucketPolicyRequesterPays(boolean requesterPays) {
            bucketOptions));
  }

+  private void testBucketPolicyV3RequesterPays(boolean requesterPays) {
+    if (requesterPays) {


Conditionals around asserts are a code smell. This should be two tests, one with and one without requesterPays.

Or two helper methods

elharo · 2020-02-11T16:17:41Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

+
+    // Remove a member
+    List<com.google.cloud.Binding> updatedBindings = new ArrayList(updatedPolicy.getBindingsList());
+    for (int i = 0; i < updatedBindings.size(); ++i) {


i++ is more common

elharo · 2020-02-11T16:19:29Z

pom.xml

@@ -63,7 +63,7 @@
    <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
    <github.global.server>github</github.global.server>
    <site.installationModule>google-cloud-storage-parent</site.installationModule>
-    <google.core.version>1.91.3</google.core.version>
+    <google.core.version>1.92.3-SNAPSHOT</google.core.version>


No snapshot dependencies.

Doing it temporary until dependency PR is merged/released.

elharo · 2020-02-27T12:33:01Z

google-cloud-storage/src/test/java/com/google/cloud/storage/it/ITStorageTest.java

@@ -2398,6 +2402,160 @@ private void testBucketPolicyRequesterPays(boolean requesterPays) {
            bucketOptions));
  }

+  private void testBucketPolicyV3RequesterPays(boolean requesterPays) {
+    if (requesterPays) {


Or two helper methods

elharo · 2020-02-27T12:33:34Z

pom.xml

@@ -63,7 +63,7 @@
    <project.reporting.outputEncoding>UTF-8</project.reporting.outputEncoding>
    <github.global.server>github</github.global.server>
    <site.installationModule>google-cloud-storage-parent</site.installationModule>
-    <google.core.version>1.92.5</google.core.version>
+    <google.core.version>1.92.6-SNAPSHOT</google.core.version>


hard veto on this; we must not depend on snapshots. If this means we have to wait for a release of google core so be it.

I'm doing this only for development*

codecov · 2020-02-27T21:56:59Z

Codecov Report

Merging #120 into master will increase coverage by 0.06%.
The diff coverage is 44.73%.

@@             Coverage Diff              @@
##             master     #120      +/-   ##
============================================
+ Coverage      63.4%   63.46%   +0.06%     
  Complexity      537      537              
============================================
  Files            30       30              
  Lines          4752     4752              
  Branches        427      427              
============================================
+ Hits           3013     3016       +3     
+ Misses         1579     1576       -3     
  Partials        160      160

Impacted Files	Coverage Δ	Complexity Δ
...rc/main/java/com/google/cloud/storage/Storage.java	`80.57% <0%> (ø)`	`0 <0> (ø)`	⬇️
...om/google/cloud/storage/spi/v1/HttpStorageRpc.java	`1.63% <0%> (ø)`	`1 <0> (ø)`	⬇️
...va/com/google/cloud/storage/spi/v1/StorageRpc.java	`64.86% <100%> (ø)`	`0 <0> (ø)`	⬇️
...in/java/com/google/cloud/storage/PolicyHelper.java	`64.86% <55.55%> (ø)`	`5 <2> (ø)`	⬇️
...gle/cloud/storage/testing/RemoteStorageHelper.java	`64.46% <0%> (+2.47%)`	`9% <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 2627a93...900a991. Read the comment docs.

frankyn · 2020-02-28T04:20:51Z

@elharo could you help cut a release for libraries-bom, @chingor13 released google-cloud-bom which updated java-core dependency to latest release which has IAM Conditions support.

It's blocking the Linkage Monitor.

Add IAM Conditions support

187e98c

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Feb 6, 2020

frankyn requested review from JesseLovelace, chingor13 and jkwlui February 6, 2020 18:38

frankyn added 3 commits February 6, 2020 10:41

format

ef9a53d

wip

f7e1609

set version in mock policies

1160ad5

This was referenced Feb 7, 2020

Iam condition samples googleapis/google-cloud-java#7127

Closed

Iam condition samples googleapis/google-cloud-java#7128

Closed

jkwlui approved these changes Feb 11, 2020

View reviewed changes

elharo suggested changes Feb 11, 2020

View reviewed changes

wip

938c9fe

frankyn requested review from elharo and removed request for chingor13 February 26, 2020 23:13

frankyn added 5 commits February 26, 2020 15:13

address comments

d7a4077

Merge branch 'master' into iam-conditions-support

f2013df

fix pom.

8ad5043

revert http-client-bom

6284c40

address feedback

7117481

elharo suggested changes Feb 27, 2020

View reviewed changes

address integration tests feedback

8606ab7

frankyn requested a review from elharo February 27, 2020 19:40

JesseLovelace approved these changes Feb 27, 2020

View reviewed changes

frankyn added 3 commits February 27, 2020 13:53

Merge branch 'master' into iam-conditions-support

cd2ead7

update core

3941f5c

lint

1dd8343

frankyn added 2 commits February 27, 2020 14:36

Merge branch 'master' into iam-conditions-support

1bd1916

clean up ubla

cff4242

frankyn added 2 commits February 27, 2020 20:09

format

95a17db

Merge branch 'master' into iam-conditions-support

900a991

frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 28, 2020

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 28, 2020

elharo approved these changes Feb 28, 2020

View reviewed changes

frankyn merged commit 8256f6d into master Feb 28, 2020

frankyn deleted the iam-conditions-support branch February 28, 2020 21:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add IAM Conditions support #120

feat: add IAM Conditions support #120

frankyn commented Feb 6, 2020 •

edited

Loading

jkwlui left a comment

elharo left a comment

elharo Feb 11, 2020

elharo Feb 11, 2020

elharo Feb 11, 2020

elharo Feb 11, 2020

elharo Feb 27, 2020

elharo Feb 11, 2020

elharo Feb 11, 2020

frankyn Feb 26, 2020

elharo Feb 27, 2020

elharo Feb 27, 2020

frankyn Feb 27, 2020

codecov bot commented Feb 27, 2020 •

edited

Loading

frankyn commented Feb 28, 2020

feat: add IAM Conditions support #120

feat: add IAM Conditions support #120

Conversation

frankyn commented Feb 6, 2020 • edited Loading

jkwlui left a comment

Choose a reason for hiding this comment

elharo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 27, 2020 • edited Loading

Codecov Report

frankyn commented Feb 28, 2020

frankyn commented Feb 6, 2020 •

edited

Loading

codecov bot commented Feb 27, 2020 •

edited

Loading