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

Implement support for IAM conditions. #2854

Merged
merged 5 commits into from
Jul 5, 2019
Merged

Conversation

dopiera
Copy link
Contributor

@dopiera dopiera commented Jul 4, 2019

This fixes #2732

This is basically a couple of helpers for constructing and printing
relevant protos.


This change is Reviewable

This fixes googleapis#2732

This is basically a couple of helpers for constructing and printing
relevant protos.
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jul 4, 2019
@codecov
Copy link

codecov bot commented Jul 4, 2019

Codecov Report

Merging #2854 into master will decrease coverage by 0.11%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2854      +/-   ##
==========================================
- Coverage   89.33%   89.22%   -0.12%     
==========================================
  Files         296      297       +1     
  Lines       19704    19736      +32     
==========================================
+ Hits        17603    17609       +6     
- Misses       2101     2127      +26
Impacted Files Coverage Δ
google/cloud/bigtable/instance_admin.h 100% <ø> (ø) ⬆️
google/cloud/bigtable/iam_binding.cc 100% <100%> (ø) ⬆️
google/cloud/bigtable/iam_binding.h 100% <100%> (ø) ⬆️
google/cloud/bigtable/instance_admin.cc 98.58% <100%> (ø) ⬆️
google/cloud/bigtable/expr.cc 100% <100%> (ø)
...gle/cloud/storage/internal/access_control_common.h 56.25% <0%> (-43.75%) ⬇️
google/cloud/bigtable/metadata_update_policy.h 71.42% <0%> (-28.58%) ⬇️
google/cloud/storage/internal/common_metadata.h 71.73% <0%> (-26.09%) ⬇️
...oogle/cloud/bigtable/internal/unary_client_utils.h 79.41% <0%> (-14.71%) ⬇️
google/cloud/storage/internal/generic_request.h 76.92% <0%> (-5.13%) ⬇️
... and 10 more

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 7f13737...8121af1. Read the comment docs.

@coryan coryan changed the title Imeplement support for IAM conditions. Implement support for IAM conditions. Jul 4, 2019
@dopiera
Copy link
Contributor Author

dopiera commented Jul 5, 2019

I've updated the ABI expectations, but TBH, I don't understand what it's complaining about.

It claims that one symbol is removed in the "Source compatibility" category (either I'm reading it wrong or it doesn't specify which one). When locally generating these reports, I get 100% compatibility and the PR doesn't remove anything, so I'll claim it a false positive.

The checker claims that google::iam::v1::_Binding_default_instance_ has
been removed, which is false.
Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

Reviewed 12 of 12 files at r1, 1 of 1 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dopiera)


google/cloud/bigtable/expr.h, line 1 at r1 (raw file):

// copyright 2019 google llc

nit: the capitalization is all wrong for the copyright boilerplate.


google/cloud/bigtable/expr.h, line 18 at r1 (raw file):

#define GOOGLE_CLOUD_CPP_GOOGLE_CLOUD_BIGTABLE_EXPR_H_

#include "google/type/expr.pb.h"

This should be <google/type/expr.pb.h>.


google/cloud/bigtable/instance_admin.h, line 907 at r1 (raw file):

   * @return Policy the full IAM policy for the instance.
   *
   * @deprecated this function is deprecated; it doesn't support conditional

We should consider using the [[deprecated]] attribute for the function. Maybe a TODO and a bug?

Copy link
Contributor Author

@dopiera dopiera left a comment

Choose a reason for hiding this comment

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

Reviewable status: 11 of 13 files reviewed, 2 unresolved discussions (waiting on @coryan)


google/cloud/bigtable/expr.h, line 18 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

This should be <google/type/expr.pb.h>.

Done.


google/cloud/bigtable/instance_admin.h, line 907 at r1 (raw file):

Previously, coryan (Carlos O'Ryan) wrote…

We should consider using the [[deprecated]] attribute for the function. Maybe a TODO and a bug?

Done.

Copy link
Contributor

@coryan coryan left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 2 of 2 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@coryan coryan merged commit 4525a9a into googleapis:master Jul 5, 2019
@dopiera dopiera deleted the conditions branch August 28, 2019 15:15
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.

Support IAM conditions in Cloud Bigtable.
3 participants