-
Notifications
You must be signed in to change notification settings - Fork 0
Skipping test cases with pap issue #51
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
base: main
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @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
🧠 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
Activity
Using Gemini Code AssistThe 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
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 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
|
There was a problem hiding this 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.
google-cloud-storage/acceptance/storage/bucket_default_acl_test.rb
Outdated
Show resolved
Hide resolved
google-cloud-storage/acceptance/storage/bucket_uniform_bucket_level_access_test.rb
Outdated
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this 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.
| 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:
bundle exec rake ciin the gem subdirectory.closes: #<issue_number_goes_here>