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

cleanup: remove remnant IAM deprecation macros #8873

Merged
merged 1 commit into from
May 4, 2022

Conversation

devbww
Copy link
Contributor

@devbww devbww commented May 4, 2022

GOOGLE_CLOUD_CPP_BIGTABLE_IAM_DEPRECATED disappeared in #8652.
GOOGLE_CLOUD_CPP_IAM_DEPRECATED disappeared in #8667.


This change is Reviewable

`GOOGLE_CLOUD_CPP_BIGTABLE_IAM_DEPRECATED` disappeared in googleapis#8652.
`GOOGLE_CLOUD_CPP_IAM_DEPRECATED` disappeared in googleapis#8667.
@google-cloud-cpp-bot
Copy link
Collaborator

Google Cloud Build Logs
For commit: 4f11776cb307120c8b5fa830bb5b9fbc4047ed27

ℹ️ NOTE: Kokoro logs are linked from "Details" below.

Copy link
Member

@dbolduc dbolduc left a comment

Choose a reason for hiding this comment

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

Oops, I missed this in #8667. Thanks for noticing and cleaning up!

@@ -82,8 +82,6 @@ elseif (GOOGLE_CLOUD_CPP_GENERATE_DOXYGEN)
# to be noops or have simple values.
set(DOXYGEN_PREDEFINED
"GOOGLE_CLOUD_CPP_DEPRECATED(x)="
"GOOGLE_CLOUD_CPP_IAM_DEPRECATED(x)="
"GOOGLE_CLOUD_CPP_BIGTABLE_IAM_DEPRECATED(x)="
"GOOGLE_CLOUD_CPP_SPANNER_ADMIN_API_DEPRECATED(x)="
Copy link
Member

Choose a reason for hiding this comment

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

Hmm.. I recently added

#define GOOGLE_CLOUD_CPP_BIGTABLE_DATA_CLIENT_DEPRECATED(name) \
GOOGLE_CLOUD_CPP_DEPRECATED( \
"google::cloud::bigtable::DataClient::" name \
" is deprecated, and will be removed on or shortly after 2023-05-01." \
" See GitHub issue #8800 for more information.")
and the doxygen seems fine (although I don't remember what the original problem was). Maybe it is fine because it reduces to GOOGLE_CLOUD_CPP_DEPRECATED? If so, then maybe we can remove the spanner macro from this list.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My understanding is that it should be a problem if doxygen saw any code that used GOOGLE_CLOUD_CPP_BIGTABLE_DATA_CLIENT_DEPRECATED. So you may want to investigate.

[The original problem is that doxygen doesn't usually expand macros during its parse, so those attribute macros look like syntax errors.]

Copy link
Member

Choose a reason for hiding this comment

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

Ack. I think the GOOGLE_CLOUD_CPP_BIGTABLE_DATA_CLIENT_DEPRECATED macro is only used in tests and internal classes. So we should be fine.

@codecov
Copy link

codecov bot commented May 4, 2022

Codecov Report

Merging #8873 (4f11776) into main (b33dcb7) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #8873   +/-   ##
=======================================
  Coverage   93.20%   93.20%           
=======================================
  Files        1474     1474           
  Lines      124487   124487           
=======================================
+ Hits       116033   116034    +1     
+ Misses       8454     8453    -1     
Impacted Files Coverage Δ
google/cloud/version.h 100.00% <ø> (ø)
google/cloud/completion_queue_test.cc 97.13% <0.00%> (+0.19%) ⬆️

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 b33dcb7...4f11776. Read the comment docs.

@devbww devbww marked this pull request as ready for review May 4, 2022 04:34
@devbww devbww requested a review from a team as a code owner May 4, 2022 04:34
@devbww devbww merged commit 331db12 into googleapis:main May 4, 2022
@devbww devbww deleted the iam-deprecated branch May 4, 2022 04:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants