Skip to content

Support bucket encryption config update#58

Open
shubhangi-google wants to merge 28 commits intomainfrom
support_bucket_encryption_config_update
Open

Support bucket encryption config update#58
shubhangi-google wants to merge 28 commits intomainfrom
support_bucket_encryption_config_update

Conversation

@shubhangi-google
Copy link
Copy Markdown
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>

Copy link
Copy Markdown

@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 introduces Bucket Encryption Enforcement Config, adding attributes to the Bucket class for managing customer-managed, customer-supplied, and Google-managed encryption restrictions. The implementation includes updated internal update logic, new sample scripts, and expanded test coverage. Review feedback identifies a logic error where using || {} prevents clearing configurations with nil, the accidental disabling of existing sample tests, a missing method referenced in unit tests, a documentation typo, and the need to remove debugging artifacts like require 'pry' and unused parameters in internal helper methods.

I am having trouble creating individual review comments. Click here to see my feedback.

google-cloud-storage/lib/google/cloud/storage/bucket.rb (770)

high

Using || {} here prevents the configuration from being cleared when nil is passed. If the intention is to allow clearing the configuration (as suggested by the documentation and the unit tests), this should allow nil to be assigned to the underlying API object. Assigning {} will likely result in an empty object being sent in the request rather than the field being omitted or cleared.

            new_customer_managed_encryption_enforcement_config

google-cloud-storage/lib/google/cloud/storage/bucket.rb (823)

high

Using || {} here prevents the configuration from being cleared when nil is passed. See the comment on line 770 for more details.

            new_customer_supplied_encryption_enforcement_config

google-cloud-storage/lib/google/cloud/storage/bucket.rb (882)

high

Using || {} here prevents the configuration from being cleared when nil is passed. See the comment on line 770 for more details.

            new_google_managed_encryption_enforcement_config

google-cloud-storage/samples/acceptance/buckets_test.rb (69-126)

high

A large portion of the existing tests has been commented out. These tests should be re-enabled to ensure no regressions were introduced and to maintain test coverage for the samples.

google-cloud-storage/test/google/cloud/storage/bucket_encryption_test.rb (172)

high

The method update_bucket_encryption_enforcement_config is called in this test but does not appear to be defined in lib/google/cloud/storage/bucket.rb. This will result in a NoMethodError when running the unit tests.

google-cloud-storage/lib/google/cloud/storage/bucket.rb (864)

medium

There is a typo in the example method name. It should be google_managed_encryption_enforcement_config instead of new_google_managed_encryption_enforcement_config.

        #   bucket.google_managed_encryption_enforcement_config = new_config

google-cloud-storage/lib/google/cloud/storage/bucket.rb (3423-3433)

medium

The addition of the bucket_encryption_config parameter to patch_gapi! appears to be unused in the provided changes and breaks the generic nature of this internal helper method. If it is not required for a specific reason, it should be removed to maintain code clarity and consistency.

                        if_metageneration_not_match: nil
          attributes = Array(attributes)
          attributes.flatten!
          return if attributes.empty?
          ensure_service!
          patch_args = attributes.to_h do |attr|
            [attr, @gapi.send(attr)]
          end

google-cloud-storage/samples/acceptance/buckets_test.rb (61)

medium

The require 'pry' statement should be removed before merging, as it is typically used for local debugging and is not appropriate for shared test code.

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