Set bucket ip filter - #65
Conversation
Removed redundant explanation about uniform bucket-level access.
Removed unnecessary line break before accessing the IP filter.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces support for IP filter configurations on Google Cloud Storage buckets, including new methods, a projection parameter to retrieve metadata, and corresponding samples and tests. The review feedback highlights several key areas for improvement: ensuring projection: "full" is used during bucket creation in tests to prevent failures, restoring accidentally deleted documentation for uniform bucket-level access, documenting the new projection parameter in YARD, fixing an unused parameter and incorrect call syntax in the listing sample, resolving a CLI argument parsing bug in the enable sample, and correcting a misleading test comment.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| let :bucket do | ||
| storage.bucket(bucket_name, projection: "full") || | ||
| storage.create_bucket(bucket_name, ip_filter: ip_filter_disabled) | ||
| end |
There was a problem hiding this comment.
The ip_filter metadata is only returned when using the full projection (as noted in the samples). If the bucket does not already exist in the project, storage.create_bucket will be called without projection: "full", meaning the returned bucket object won't have ip_filter populated. This will cause the assertion _(bucket.ip_filter).wont_be_nil to fail on a clean run. Adding projection: "full" to create_bucket ensures the test is robust and passes on clean runs.
let :bucket do
storage.bucket(bucket_name, projection: "full") ||
storage.create_bucket(bucket_name, ip_filter: ip_filter_disabled, projection: "full")
end| # Sets whether uniform bucket-level access is enabled for this bucket. | ||
| # | ||
| # Before enabling uniform bucket-level access please review [uniform bucket-level | ||
| # access](https://cloud.google.com/storage/docs/uniform-bucket-level-access). |
There was a problem hiding this comment.
The detailed documentation explaining the implications of enabling uniform bucket-level access (such as IAM configuration, legacy ACL policies, and locking behavior) was removed. This documentation is highly valuable for users. Please revert this deletion to preserve the detailed explanation.
# Sets whether uniform bucket-level access is enabled for this bucket. When this is enabled, access to the
# bucket will be configured through IAM, and legacy ACL policies will not work. When it is first enabled,
# {#uniform_bucket_level_access_locked_at} will be set by the API automatically. The uniform bucket-level access
# can then be disabled until the time specified, after which it will become immutable and calls to change it
# will fail. If uniform bucket-level access is enabled, calls to access legacy ACL information will fail.
#
# Before enabling uniform bucket-level access please review [uniform bucket-level
# access](https://cloud.google.com/storage/docs/uniform-bucket-level-access).| hierarchical_namespace: nil | ||
| hierarchical_namespace: nil, | ||
| ip_filter: nil, | ||
| projection: nil |
There was a problem hiding this comment.
The new projection parameter is added to create_bucket (and other methods like bucket and buckets), but it is not documented in the YARD comments. Please add @param [String] projection to the YARD documentation for these methods to ensure clarity for users.
| def list_bucket_ip_filters bucket_name | ||
| # The ID of your GCP project | ||
| # project_id = "your-project-id" | ||
|
|
||
| require "google/cloud/storage" | ||
|
|
||
| storage = Google::Cloud::Storage.new | ||
|
|
||
| puts "Buckets:" | ||
| # Use projection: "full" to ensure IP filter metadata is returned | ||
| storage.buckets(projection: "full").all do |bucket| | ||
| ip_filter = bucket.ip_filter | ||
| mode = ip_filter ? ip_filter.mode : "Not Configured" | ||
|
|
||
| puts "Bucket Name: #{bucket.name}, IP Filtering Mode: #{mode}" | ||
| end | ||
| end | ||
| # [END storage_list_bucket_ip_filters] | ||
|
|
||
| if $PROGRAM_NAME == __FILE__ | ||
| list_bucket_ip_filters bucket_name: ARGV.shift |
There was a problem hiding this comment.
The bucket_name parameter is completely unused in list_bucket_ip_filters because the method lists all buckets in the project. Additionally, calling it with bucket_name: ARGV.shift passes a Hash to a positional parameter in Ruby 3.x, which is incorrect and confusing. Removing the parameter entirely simplifies the method and fixes the incorrect call syntax.
def list_bucket_ip_filters
# The ID of your GCP project
# project_id = "your-project-id"
require "google/cloud/storage"
storage = Google::Cloud::Storage.new
puts "Buckets:"
# Use projection: "full" to ensure IP filter metadata is returned
storage.buckets(projection: "full").all do |bucket|
ip_filter = bucket.ip_filter
mode = ip_filter ? ip_filter.mode : "Not Configured"
puts "Bucket Name: #{bucket.name}, IP Filtering Mode: #{mode}"
end
end
# [END storage_list_bucket_ip_filters]
if $PROGRAM_NAME == __FILE__
list_bucket_ip_filters
end| # Lists IP filter configurations for buckets in the project | ||
| retry_resource_exhaustion do | ||
| out, _err = capture_io do | ||
| list_bucket_ip_filters bucket_name |
There was a problem hiding this comment.
Update the call to list_bucket_ip_filters to match the updated method signature that does not require the unused bucket_name parameter.
list_bucket_ip_filters| if $PROGRAM_NAME == __FILE__ | ||
| enable_bucket_ip_filter bucket_name: ARGV.shift, mode: ARGV.shift | ||
| end |
There was a problem hiding this comment.
If the sample is run from the CLI without a second argument (e.g., ruby storage_enable_bucket_ip_filter.rb my-bucket), ARGV.shift for mode returns nil. This overrides the default keyword argument mode: "Enabled" to nil, which will cause an API error since mode is a required field. Only passing mode if it is provided prevents this issue.
if $PROGRAM_NAME == __FILE__
bucket_name = ARGV.shift
mode = ARGV.shift
kwargs = { bucket_name: bucket_name }
kwargs[:mode] = mode if mode
enable_bucket_ip_filter(**kwargs)
end| end | ||
| end | ||
|
|
||
| # Enables IP filter of an existing bucket (SKIPPED) |
There was a problem hiding this comment.
The comment says (SKIPPED) but the test is actually executed with mode: "Disabled". Updating the comment to accurately describe why it is run with mode: "Disabled" (to avoid blocking test runner access) improves maintainability and clarity.
# Enables IP filter of an existing bucket with "Disabled" mode to avoid blocking test runner access| gapi = @service.list_buckets prefix: @prefix, token: @token, | ||
| max: @max, user_project: @user_project, | ||
| soft_deleted: @soft_deleted | ||
| soft_deleted: @soft_deleted, |
There was a problem hiding this comment.
soft delete changes came here
2 participants
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>