Skip to content

Conversation

@wolf31o2
Copy link
Member

@wolf31o2 wolf31o2 commented Dec 28, 2025

Closes #1150

Summary by cubic

Removed deprecated GCS credentials file support from the blob store and now rely on Application Default Credentials (ADC). This cleans up options, flags, and tests, and simplifies client initialization.

  • Refactors

    • Removed credentialsFile field, validation, WithCredentialsFile option, and --credentials-file CLI flag.
    • Simplified GCS client setup to use ADC only.
    • Deleted related unit tests and unused code.
  • Migration

    • Remove any use of --credentials-file and WithCredentialsFile().
    • Ensure ADC is set up (e.g., GOOGLE_APPLICATION_CREDENTIALS or workload identity).

Written for commit 013d75b. Summary will update automatically on new commits.

Summary by CodeRabbit

  • Breaking Changes

    • Removed credentials file configuration option from GCS blob storage. Users can no longer pass a credentials file path for authentication.
  • Dependencies

    • Updated Google Cloud and authentication libraries (Google API, oauth2, Protocol Buffers).

✏️ Tip: You can customize this high-level summary in your review settings.

@wolf31o2 wolf31o2 requested a review from a team as a code owner December 28, 2025 15:48
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 28, 2025

📝 Walkthrough

Walkthrough

This pull request removes GCS credentials file functionality from the BlobStore plugin. The changes eliminate the credentialsFile field from the BlobStoreGCS struct, remove the WithCredentialsFile() option constructor, delete credentials validation and CLI flag handling in the plugin layer, and remove associated tests. Additionally, several dependencies are updated to newer versions.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Pre-merge checks and finishing touches

✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The pull request title accurately summarizes the main change: removing deprecated credentials file functionality from the GCS blob store implementation across multiple files.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/remove-gcs-credentials-file

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

No issues found across 7 files

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
database/plugin/blob/gcs/database.go (1)

433-467: Documentation and code mismatch: README documents credentials-file option but it's not implemented in the plugin.

The Start() method correctly uses Application Default Credentials (ADC) with no explicit credentials handling. However, the README documents credentials-file as an optional GCS configuration parameter, but this option is not defined in the plugin's command-line options (plugin.go lines 46-54) and is not handled anywhere in the code. Either remove credentials-file from the README documentation, or implement support for loading credentials from an explicit file path via the option.WithCredentials() client option. Users following the README will be unable to use the documented credentials-file parameter.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e737550 and 013d75b.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • database/plugin/blob/gcs/database.go
  • database/plugin/blob/gcs/options.go
  • database/plugin/blob/gcs/options_test.go
  • database/plugin/blob/gcs/plugin.go
  • database/plugin/blob/gcs/plugin_test.go
  • go.mod
💤 Files with no reviewable changes (3)
  • database/plugin/blob/gcs/options.go
  • database/plugin/blob/gcs/options_test.go
  • database/plugin/blob/gcs/plugin_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-11-20T19:58:38.889Z
Learnt from: wolf31o2
Repo: blinklabs-io/dingo PR: 0
File: :0-0
Timestamp: 2025-11-20T19:58:38.889Z
Learning: In Go, test dependencies (such as github.com/stretchr/testify) are correctly marked as direct dependencies in go.mod, not as indirect dependencies, even when they are only used in test files. This is a standard practice in Go's module system.

Applied to files:

  • go.mod
🧬 Code graph analysis (1)
database/plugin/blob/gcs/database.go (1)
database/plugin/blob/gcs/logger.go (1)
  • GcsLogger (24-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
  • GitHub Check: go-test (1.24.x, windows-latest)
  • GitHub Check: go-test (1.25.x, windows-latest)
  • GitHub Check: go-test (1.25.x, ubuntu-latest)
  • GitHub Check: go-test (1.24.x, macos-latest)
  • GitHub Check: go-test (1.25.x, macos-latest)
  • GitHub Check: go-test (1.24.x, ubuntu-latest)
  • GitHub Check: lint
  • GitHub Check: docker (ubuntu-latest, amd64)
  • GitHub Check: nilaway
  • GitHub Check: Analyze (go)
🔇 Additional comments (4)
database/plugin/blob/gcs/database.go (1)

36-43: LGTM! Clean removal of credentials file field.

The BlobStoreGCS struct has been simplified by removing the credentialsFile field. The remaining fields are well-organized and sufficient for the blob store operations.

database/plugin/blob/gcs/plugin.go (2)

24-26: LGTM! Credentials option removed from CLI configuration.

The cmdlineOptions struct has been simplified to only include the bucket name. This is consistent with the removal of credentials file handling throughout the codebase.


59-73: LGTM! Plugin initialization simplified.

NewFromCmdlineOptions() now only passes the bucket configuration to the blob store. Authentication will be handled automatically through Application Default Credentials (ADC), which is the recommended approach for GCS client libraries.

go.mod (1)

38-38: LGTM! Routine dependency updates supporting Google Cloud client libraries.

The dependency bumps are routine version updates to google.golang.org/api v0.258.0, golang.org/x/oauth2 v0.34.0, google.golang.org/genproto/googleapis/rpc, and google.golang.org/protobuf v1.36.11. These updates maintain compatibility with the existing GCS plugin, which now uses Application Default Credentials when a credentials file is not explicitly provided.

@wolf31o2 wolf31o2 merged commit 9cb7655 into main Dec 28, 2025
15 of 16 checks passed
@wolf31o2 wolf31o2 deleted the fix/remove-gcs-credentials-file branch December 28, 2025 15:55
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.

3 participants