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: add IAM Conditions support #120

Merged
merged 18 commits into from Feb 28, 2020
Merged

feat: add IAM Conditions support #120

merged 18 commits into from Feb 28, 2020

Conversation

frankyn
Copy link
Member

@frankyn frankyn commented Feb 6, 2020

Add IAM Conditions Support.

Depends on:
googleapis/java-core#110

Pending Review List:

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

@jkwlui jkwlui left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@elharo elharo left a comment

Choose a reason for hiding this comment

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

Mostly nits, but snapshot dependencies are a hard no.

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

Choose a reason for hiding this comment

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

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


assertEquals(libPolicy, actualLibPolicy);
assertTrue(new ApiPolicyMatcher(apiPolicy).matches(actualApiPolicy));
// Policy actualLibPolicy = PolicyHelper.convertFromApiPolicy(apiPolicy);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

don't comment out code

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

private void testBucketPolicyV3RequesterPays(boolean requesterPays) {
if (requesterPays) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Or two helper methods


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

Choose a reason for hiding this comment

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

i++ is more common

pom.xml Outdated
@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

No snapshot dependencies.

Copy link
Member Author

Choose a reason for hiding this comment

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

Doing it temporary until dependency PR is merged/released.

@frankyn frankyn requested review from elharo and removed request for chingor13 February 26, 2020 23:13
@@ -2398,6 +2402,160 @@ private void testBucketPolicyRequesterPays(boolean requesterPays) {
bucketOptions));
}

private void testBucketPolicyV3RequesterPays(boolean requesterPays) {
if (requesterPays) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Or two helper methods

pom.xml Outdated
@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doing this only for development*

@frankyn frankyn requested a review from elharo February 27, 2020 19:40
@codecov
Copy link

codecov bot commented Feb 27, 2020

Codecov Report

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

Impacted file tree graph

@@             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
Copy link
Member Author

frankyn commented Feb 28, 2020

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

@frankyn frankyn added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 28, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 28, 2020
@frankyn frankyn merged commit 8256f6d into master Feb 28, 2020
@frankyn frankyn deleted the iam-conditions-support branch February 28, 2020 21:12
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.

None yet

6 participants