Skip to content

Conversation

@Bencheng21
Copy link
Contributor

@Bencheng21 Bencheng21 commented Nov 14, 2025

Description

in c1, the field name is oauth2, we should use the same name in connector to guarantee smooth migration.

  • Bug fix
  • New feature

Useful links:

Summary by CodeRabbit

  • Chores
    • Updated OAuth2 configuration field identifier from "oauth2-token" to "oauth2".

@coderabbitai
Copy link

coderabbitai bot commented Nov 14, 2025

Walkthrough

The Oauth2TokenField configuration field initialization parameter in pkg/config/config.go was modified from "oauth2-token" to "oauth2". This changes the internal key used for the OAuth2 token field within the configuration schema.

Changes

Cohort / File(s) Summary
Configuration Schema Updates
pkg/config/config.go
Changed Oauth2TokenField initialization parameter from "oauth2-token" to "oauth2"

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

  • Verify the key change is intentional and doesn't break existing configuration parsing or storage
  • Confirm no other references depend on the "oauth2-token" key name

Possibly related PRs

Suggested reviewers

  • btipling
  • kans

Poem

🐰 From "token-oauth" we did pare,
Now "oauth" floats light as air,
Shorter keys, cleaner sight,
Configuration feels just right! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title check ⚠️ Warning The title mentions updating oauth displayname, but the actual change is renaming the oauth2-token field key to oauth2, not updating a display name. Revise the title to accurately reflect the main change, such as 'Change oauth2 field key from oauth2-token to oauth2' or similar to better represent the actual modification.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch ben/oauth

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3f565d4 and 9380883.

📒 Files selected for processing (1)
  • pkg/config/config.go (1 hunks)
🔇 Additional comments (1)
pkg/config/config.go (1)

37-41: Verify breaking change impact and migration path.

The field identifier change from "oauth2-token" to "oauth2" aligns with the PR objective of matching c1 naming conventions. However, this is a breaking change that will affect CLI flags, configuration files, and any code referencing this field.

Please verify the following:

  1. Backward compatibility: Ensure existing configurations using --oauth2-token or oauth2-token key are handled gracefully, or that users are properly notified of the required change.

  2. Complete migration: Run the following script to check for any remaining references to the old field name:

  3. Field name conflicts: Verify there are no other fields already using "oauth2" as an identifier in the broader baton ecosystem.

  4. SDK compatibility: Confirm this field name matches the expected identifier in baton-sdk and the c1 platform.


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


Oauth2TokenField = field.Oauth2Field(
"oauth2-token",
"oauth2",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

c1 uses the field name oauth2. We should use the same name in the connector to ensure a smooth migration, so customers won’t need to manually click the blue button to re-authenticate.

@Bencheng21
Copy link
Contributor Author

Given this connector is not enabled in lambda yet, I think we can merge this one.

@Bencheng21 Bencheng21 requested a review from kans November 17, 2025 17:32
@Bencheng21 Bencheng21 closed this Nov 17, 2025
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.

2 participants