Skip to content

Conversation

@filippolmt
Copy link
Contributor

@filippolmt filippolmt commented Jun 18, 2025

PR Type

Enhancement


Description

• Replace gsutil commands with gcloud storage equivalents
• Update ACL setting syntax to use predefined-acl flags
• Improve shell scripting with modern bash practices
• Streamline bucket listing and error handling logic


Changes walkthrough 📝

Relevant files
Enhancement
set-bucket-acls.sh
Migrate gsutil to gcloud storage commands                               

scripts/set-bucket-acls.sh

• Replace gsutil ls -d with gcloud storage ls for bucket checking

Update ACL commands from gsutil acl set to gcloud storage objects
update --predefined-acl
• Improve shell scripting with modern quoting
and syntax patterns
• Simplify bucket list formatting and file
initialization

+15/-20 

Need help?
  • Type /help how to ... in the comments thread for any questions about PR-Agent usage.
  • Check out the documentation for more information.
  • @filippolmt filippolmt requested a review from Copilot June 18, 2025 08:09
    Copy link

    Copilot AI left a comment

    Choose a reason for hiding this comment

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

    Pull Request Overview

    This PR refactors the bucket ACL update script to complete the migration from gsutil to gcloud storage commands.

    • Replaces gsutil commands with equivalent gcloud storage commands for listing and updating ACLs.
    • Refactors bucket list formatting and file cleanup syntax, and updates the DRY_RUN conditional check for improved consistency.
    Comments suppressed due to low confidence (1)

    scripts/set-bucket-acls.sh:25

    • Adding a brief comment explaining the DRY_RUN check could improve clarity for future maintainers.
      if [[ "$DRY_RUN" -ne 0 ]]; then
    

    Comment on lines +11 to +15
    BUCKETS="put-here-the-first-bucket put-here-the-second-bucket"

    echo -n "" >"${NOT_PROCESSED_BUCKETS_FILE}"
    for BUCKET in ${BUCKETS}; do
    : >"$NOT_PROCESSED_BUCKETS_FILE"

    for BUCKET in $BUCKETS; do
    Copy link

    Copilot AI Jun 18, 2025

    Choose a reason for hiding this comment

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

    [nitpick] Consider handling bucket names safely by either quoting the variable or using an array if bucket names might include spaces.

    Copilot uses AI. Check for mistakes.
    @sparkfabrik-ai-bot
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Variable Quoting

    The BUCKETS variable is not properly quoted in the for loop, which could cause issues if bucket names contain spaces or special characters. The variable should be quoted or converted to an array.

    for BUCKET in $BUCKETS; do
    Path Globbing

    The gcloud storage commands use shell globbing patterns that may not work as expected. The asterisk patterns should be properly quoted or escaped to ensure correct behavior with gcloud storage.

    gcloud storage objects update --recursive --predefined-acl=private "gs://${BUCKET}/*"
    gcloud storage objects update --recursive --predefined-acl=publicRead "gs://${BUCKET}/public/**"
    gcloud storage objects update --recursive --predefined-acl=private "gs://${BUCKET}/private/**"

    @filippolmt
    Copy link
    Contributor Author

    /improve

    @sparkfabrik-ai-bot
    Copy link

    sparkfabrik-ai-bot bot commented Jun 18, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 564c0ae

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix ACL naming convention

    The ACL value should be public-read instead of publicRead to match Google Cloud
    Storage's predefined ACL naming convention. The camelCase format may not be
    recognized.

    scripts/set-bucket-acls.sh [33]

    -gcloud storage objects update --recursive --predefined-acl=publicRead "gs://${BUCKET}/public/**"
    +gcloud storage objects update --recursive --predefined-acl=public-read "gs://${BUCKET}/public/**"
    Suggestion importance[1-10]: 8

    __

    Why: This is a critical fix. The gcloud storage command uses public-read (with hyphen) as the correct predefined ACL value, not publicRead (camelCase). This would cause the command to fail.

    Medium

    Previous suggestions

    Suggestions up to commit 564c0ae
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix predefined ACL value

    The ACL value publicRead should be public-read to match the standard Google Cloud
    Storage predefined ACL naming convention. The current value may cause the command to
    fail.

    scripts/set-bucket-acls.sh [33]

    -gcloud storage objects update --recursive --predefined-acl=publicRead "gs://${BUCKET}/public/**"
    +gcloud storage objects update --recursive --predefined-acl=public-read "gs://${BUCKET}/public/**"
    Suggestion importance[1-10]: 8

    __

    Why: This addresses a potential command failure issue. The correct predefined ACL value for gcloud storage commands is public-read, not publicRead, which could cause the ACL setting to fail.

    Medium
    General
    Quote variable to handle spaces

    The variable $BUCKETS should be quoted to handle bucket names with spaces correctly.
    Without quotes, bucket names containing spaces will be split into separate
    arguments.

    scripts/set-bucket-acls.sh [15]

    -for BUCKET in $BUCKETS; do
    +for BUCKET in "$BUCKETS"; do
    Suggestion importance[1-10]: 4

    __

    Why: While quoting variables is generally good practice, the suggestion is incorrect for this use case. The $BUCKETS variable contains multiple bucket names that need to be split for iteration, so quoting would prevent proper splitting.

    Low

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    2 participants