Skip to content

Conversation

@shubhangi-google
Copy link
Owner

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea.
  • Follow the instructions in CONTRIBUTING. Most importantly, ensure the tests and linter pass by running bundle exec rake ci in the gem subdirectory.
  • Update code documentation if necessary.

closes: #<issue_number_goes_here>

@gemini-code-assist
Copy link

Summary of Changes

Hello @shubhangi-google, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses failing acceptance tests in the 'google-cloud-storage' gem. Due to a recent update in Google Cloud Storage's Public Access Prevention (PAP) enforcement, certain operations involving public ACLs ('allUsers' or 'allAuthenticatedUsers') are now disallowed when PAP is active. To ensure the test suite remains stable, several tests that attempt these operations have been temporarily skipped with a clear explanation and a link to the relevant GCS documentation.

Highlights

  • Test Skipping for GCS Behavior Change: Multiple acceptance tests across various Google Cloud Storage client components are now temporarily skipped.
  • Reason for Skipping: This action is a direct response to a recent change in GCS behavior that disallows copying files with ACLs (Access Control Lists) granting access to 'allUsers' or 'allAuthenticatedUsers' when Public Access Prevention (PAP) is enforced.
  • Affected Test Categories: The skipped tests specifically target functionalities related to bucket ACLs, default bucket ACLs, uniform bucket-level access, file ACLs, file copying, and IAM settings, all of which are impacted by the new PAP enforcement.

🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console.

Changelog
  • google-cloud-storage/acceptance/storage/bucket_acl_test.rb
    • Added 'skip' directives to three test cases: 'updates predefined rules', 'deletes rules', and 'sets predefined ACL rules'.
  • google-cloud-storage/acceptance/storage/bucket_default_acl_test.rb
    • Added 'skip' directives to three test cases: 'updates predefined rules', 'deletes rules', and 'sets predefined ACL rules'.
  • google-cloud-storage/acceptance/storage/bucket_uniform_bucket_level_access_test.rb
    • Added 'skip' directives to eight test cases related to uniform bucket-level access and deprecated 'policy_only' settings.
  • google-cloud-storage/acceptance/storage/file_acl_test.rb
    • Added a 'skip' directive to the 'before' block, effectively skipping all tests within this file.
  • google-cloud-storage/acceptance/storage/file_test.rb
    • Added 'skip' directives to three test cases related to copying existing files.
  • google-cloud-storage/samples/acceptance/files_test.rb
    • Added a 'skip' directive to the 'make_public' test case.
  • google-cloud-storage/samples/acceptance/iam_test.rb
    • Added a 'skip' directive to the 'set_bucket_public_iam' test case.
Activity
  • No human activity (comments, reviews, etc.) has been recorded for this pull request yet.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request skips a number of tests due to a change in GCS behavior regarding public access prevention. While this is a necessary temporary measure, the skip message is duplicated across numerous tests and files. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, I recommend extracting this message into a single constant defined in a shared test helper (e.g., spec_helper.rb or a new constants file). This will make it much easier to manage these skipped tests, for example, if the message needs updating or when it's time to re-enable them.

@shubhangi-google
Copy link
Owner Author

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request skips a number of acceptance tests related to public access control (ACLs, IAM) due to a recent change in Google Cloud Storage behavior regarding Public Access Prevention. This is a reasonable temporary measure to get the test suite passing. My review includes a suggestion to avoid code duplication for the skip message constant, and a minor style fix.

Comment on lines +27 to +30
PAP_SKIP_MESSAGE = "Skipping this test due to a change in GCS behavior that disallows copying " \
"files with ACLs that include allUsers or allAuthenticatedUsers when public " \
"access prevention is enforced. See " \
"https://cloud.google.com/storage/docs/public-access-prevention for more details.".freeze

Choose a reason for hiding this comment

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

medium

The PAP_SKIP_MESSAGE constant is also defined in google-cloud-storage/acceptance/storage_helper.rb. To follow the DRY (Don't Repeat Yourself) principle and improve maintainability, please remove this duplicated definition. You can instead add require_relative '../../acceptance/storage_helper' at the top of this file to use the constant defined there.

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.

1 participant