Uh oh!
There was an error while loading. Please reload this page.
feat: add IAM Conditions support - #120
Conversation
jkwlui
left a comment
There was a problem hiding this comment.
Surface looks good. LGTM!
I'll let @chingor13 and others review Java aspects.
elharo
left a comment
There was a problem hiding this comment.
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()) { |
There was a problem hiding this comment.
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); |
There was a problem hiding this comment.
delete it if it isn't relevant. or fix it if it is.
| public void testBucketPolicy() { | ||
| testBucketPolicyRequesterPays(true); | ||
| testBucketPolicyRequesterPays(false); | ||
| // testBucketPolicyV3RequesterPays(true); |
| } | ||
| private void testBucketPolicyV3RequesterPays(boolean requesterPays) { | ||
| if (requesterPays) { |
There was a problem hiding this comment.
Conditionals around asserts are a code smell. This should be two tests, one with and one without requesterPays.
| // Remove a member | ||
| List<com.google.cloud.Binding> updatedBindings = new ArrayList(updatedPolicy.getBindingsList()); | ||
| for (int i = 0; i < updatedBindings.size(); ++i) { |
| <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> |
There was a problem hiding this comment.
Doing it temporary until dependency PR is merged/released.
| } | ||
| private void testBucketPolicyV3RequesterPays(boolean requesterPays) { | ||
| if (requesterPays) { |
| <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> |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm doing this only for development*
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
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. |
Add IAM Conditions Support.
Depends on:
googleapis/java-core#110
Pending Review List: