Conversation
WalkthroughThis PR consolidates per-connector CLI handlers into unified controllers. It removes ~40 connector-specific config/install files and adds generic controllers that handle both Payments API v3 and v1/v2 flows. Infrastructure improvements include raw HTTP client bundling, v3 config retrieval refactoring, and version normalization. ChangesUnified Connector Management
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/payments/connectors/configs/updateconfig.go (1)
158-165: ⚡ Quick winRedundant authentication call.
LoadAndAuthenticateCurrentProfileis already called inRun()at lines 65-68. Calling it again here is wasteful and could trigger unnecessary token refreshes. Pass the already-resolvedprofile,profileName, andrelyingPartyas parameters torunV1Typedinstead.♻️ Proposed refactor
-func (c *ConnectorUpdateConfigController) runV1Typed( +func (c *ConnectorUpdateConfigController) runV1Typed( cmd *cobra.Command, + relyingParty client.RelyingParty, + profileName string, + profile fctl.Profile, connectorName, connectorID, script string, ) (fctl.Renderable, error) { - _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) - if err != nil { - return nil, err - } - sc, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile) + sc, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, profile) if err != nil { return nil, err }And update the call site in
Run():default: if connectorID == "" { return nil, fmt.Errorf("--connector-id is required") } - return c.runV1Typed(cmd, connectorName, connectorID, script) + return c.runV1Typed(cmd, relyingParty, profileName, *profile, connectorName, connectorID, script) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/payments/connectors/configs/updateconfig.go` around lines 158 - 165, The code calls LoadAndAuthenticateCurrentProfile again inside runV1Typed which is redundant; change runV1Typed to accept the already-resolved profile, profileName, and relyingParty (from Run where LoadAndAuthenticateCurrentProfile is first called) and remove the second LoadAndAuthenticateCurrentProfile invocation, then use those passed-in values when creating the stack client via fctl.NewStackClientFromFlags; finally update the Run() call site to pass profile, profileName, and relyingParty into runV1Typed so the function reuses the authenticated context instead of re-authenticating.cmd/payments/connectors/install/install.go (1)
150-158: ⚡ Quick winRedundant authentication call.
Same issue as in
updateconfig.go-LoadAndAuthenticateCurrentProfileis already called inRun()at lines 52-55. Pass the resolved values as parameters instead.♻️ Proposed refactor
-func (c *ConnectorInstallController) runV1Typed( +func (c *ConnectorInstallController) runV1Typed( cmd *cobra.Command, + relyingParty client.RelyingParty, + profileName string, + profile fctl.Profile, connectorName, script string, ) (fctl.Renderable, error) { - _, profile, profileName, relyingParty, err := fctl.LoadAndAuthenticateCurrentProfile(cmd) - if err != nil { - return nil, err - } - - stackClient, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile) + stackClient, err := fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, profile) if err != nil { return nil, err }And update the call site in
Run():default: - return c.runV1Typed(cmd, connectorName, script) + return c.runV1Typed(cmd, relyingParty, profileName, *profile, connectorName, script) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/payments/connectors/install/install.go` around lines 150 - 158, Remove the redundant LoadAndAuthenticateCurrentProfile call inside the installer function and instead use the already-resolved values from Run(); specifically, delete the extra call that assigns _, profile, profileName, relyingParty, err and pass the existing profile, profileName and relyingParty into fctl.NewStackClientFromFlags where it's invoked now. Update the function signature or call site in Run() so the function receives (or closes over) profile, profileName and relyingParty and then call fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(), profileName, *profile) without re-authenticating.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/payments/connectors/configs/updateconfig.go`:
- Around line 158-165: The code calls LoadAndAuthenticateCurrentProfile again
inside runV1Typed which is redundant; change runV1Typed to accept the
already-resolved profile, profileName, and relyingParty (from Run where
LoadAndAuthenticateCurrentProfile is first called) and remove the second
LoadAndAuthenticateCurrentProfile invocation, then use those passed-in values
when creating the stack client via fctl.NewStackClientFromFlags; finally update
the Run() call site to pass profile, profileName, and relyingParty into
runV1Typed so the function reuses the authenticated context instead of
re-authenticating.
In `@cmd/payments/connectors/install/install.go`:
- Around line 150-158: Remove the redundant LoadAndAuthenticateCurrentProfile
call inside the installer function and instead use the already-resolved values
from Run(); specifically, delete the extra call that assigns _, profile,
profileName, relyingParty, err and pass the existing profile, profileName and
relyingParty into fctl.NewStackClientFromFlags where it's invoked now. Update
the function signature or call site in Run() so the function receives (or closes
over) profile, profileName and relyingParty and then call
fctl.NewStackClientFromFlags(cmd, relyingParty, fctl.NewPTermDialog(),
profileName, *profile) without re-authenticating.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e81cb2a4-aa14-4788-b804-91ddbf97e046
📒 Files selected for processing (46)
Justfilecmd/payments/connectors/configs.gocmd/payments/connectors/configs/adyen.gocmd/payments/connectors/configs/atlar.gocmd/payments/connectors/configs/bankingcircle.gocmd/payments/connectors/configs/coinbaseprime.gocmd/payments/connectors/configs/column.gocmd/payments/connectors/configs/currencycloud.gocmd/payments/connectors/configs/fireblocks.gocmd/payments/connectors/configs/getconfig.gocmd/payments/connectors/configs/increase.gocmd/payments/connectors/configs/mangopay.gocmd/payments/connectors/configs/modulr.gocmd/payments/connectors/configs/moneycorp.gocmd/payments/connectors/configs/plaid.gocmd/payments/connectors/configs/powens.gocmd/payments/connectors/configs/qonto.gocmd/payments/connectors/configs/root.gocmd/payments/connectors/configs/stripe.gocmd/payments/connectors/configs/tink.gocmd/payments/connectors/configs/updateconfig.gocmd/payments/connectors/configs/wise.gocmd/payments/connectors/install/adyen.gocmd/payments/connectors/install/atlar.gocmd/payments/connectors/install/bankingcircle.gocmd/payments/connectors/install/coinbaseprime.gocmd/payments/connectors/install/column.gocmd/payments/connectors/install/currencycloud.gocmd/payments/connectors/install/fireblocks.gocmd/payments/connectors/install/generic.gocmd/payments/connectors/install/increase.gocmd/payments/connectors/install/install.gocmd/payments/connectors/install/mangopay.gocmd/payments/connectors/install/modulr.gocmd/payments/connectors/install/moneycorp.gocmd/payments/connectors/install/plaid.gocmd/payments/connectors/install/powens.gocmd/payments/connectors/install/qonto.gocmd/payments/connectors/install/root.gocmd/payments/connectors/install/stripe.gocmd/payments/connectors/install/tink.gocmd/payments/connectors/install/wise.gocmd/payments/connectors/list.gocmd/payments/connectors/root.gocmd/payments/versions/versions.gopkg/clients.go
💤 Files with no reviewable changes (36)
- cmd/payments/connectors/install/bankingcircle.go
- cmd/payments/connectors/install/modulr.go
- cmd/payments/connectors/configs/fireblocks.go
- cmd/payments/connectors/configs/column.go
- cmd/payments/connectors/configs/atlar.go
- cmd/payments/connectors/install/generic.go
- cmd/payments/connectors/install/mangopay.go
- cmd/payments/connectors/install/fireblocks.go
- cmd/payments/connectors/install/qonto.go
- cmd/payments/connectors/configs/qonto.go
- cmd/payments/connectors/install/tink.go
- cmd/payments/connectors/configs/bankingcircle.go
- cmd/payments/connectors/install/wise.go
- cmd/payments/connectors/install/moneycorp.go
- cmd/payments/connectors/configs/powens.go
- cmd/payments/connectors/configs/increase.go
- cmd/payments/connectors/configs/root.go
- cmd/payments/connectors/configs/stripe.go
- cmd/payments/connectors/configs/modulr.go
- cmd/payments/connectors/configs/adyen.go
- cmd/payments/connectors/install/increase.go
- cmd/payments/connectors/configs/mangopay.go
- cmd/payments/connectors/configs/tink.go
- cmd/payments/connectors/configs/currencycloud.go
- cmd/payments/connectors/configs/plaid.go
- cmd/payments/connectors/install/currencycloud.go
- cmd/payments/connectors/install/plaid.go
- cmd/payments/connectors/install/stripe.go
- cmd/payments/connectors/install/column.go
- cmd/payments/connectors/install/atlar.go
- cmd/payments/connectors/configs/coinbaseprime.go
- cmd/payments/connectors/configs/wise.go
- cmd/payments/connectors/install/powens.go
- cmd/payments/connectors/install/adyen.go
- cmd/payments/connectors/install/coinbaseprime.go
- cmd/payments/connectors/configs/moneycorp.go
fguery
left a comment
fguery
left a comment
There was a problem hiding this comment.
Not sure how easy it'd be, but ti'd be nice to add some tests. I know we don't have much in fctl though so up to you.
|
|
||
| rawHTTPClient := oauth2.NewClient(baseCtx, tokenSource) | ||
| rawHTTPClient.Transport = newInjectHTTPHeadersRoundTripper( | ||
| http.Header{"User-Agent": []string{fmt.Sprintf("fctl/%s", getVersion(cmd))}}, |
There was a problem hiding this comment.
Ok so the effect of that is that we would set the traceparent header and the otel collectors on the receiving end of those requests will try to set those trace ids as the parent spans. However we have no way to collect traces from fctl, so all requests made from this fctl client will have missing root spans in Signoz.
I'm pretty sure we don't want that.
| NewTinkCommand(), | ||
| NewPlaidCommand(), | ||
| ), | ||
| fctl.WithShortDescription("Install a connector"), |
There was a problem hiding this comment.
Possibly worth adding a link to the doc for the list?
There was a problem hiding this comment.
I added a command list-available that dynamically fetches the connectors and their configuration template
| name := strings.ToLower(connectorName) | ||
| switch name { | ||
| case internal.AdyenConnector: | ||
| var config shared.AdyenConfig |
There was a problem hiding this comment.
maybe we can do a refactor to avoid duplicating this that much? Even if we need to use a weaker type, it might be justified here?
There was a problem hiding this comment.
Since this is legacy code should we go to such an effort? I'd rather not spend hours debugging every single connector to ensure we made it compatible (the strongly typed SDK at least spares me that effort)
| name := strings.ToLower(connectorName) | ||
| switch name { | ||
| case internal.AdyenConnector: | ||
| config := &shared.AdyenConfig{} |
There was a problem hiding this comment.
same as on the install conector, it'd be nice to refactor that a bit
| sort.Strings(connectorNames) | ||
| for _, connectorName := range connectorNames { | ||
| fieldMap := data[connectorName] | ||
| fieldNames := make([]string, 0, len(fieldMap)) |
There was a problem hiding this comment.
I think you can use data[connectorName].Keys, depending of the go version.
Not a big deal
There was a problem hiding this comment.
but I'm old and set in my ways 😂
There was a problem hiding this comment.
Imagine my surprise, as an old Java guy, when I saw no generics, no streaming API, etc 😂
Fine :)
|
great stuff @laouji |
3 participants
As long as the stack supports it you can now find and use new connector types without updating fctl.
naturally stacks that have payments v2 still use the SDK and only support old connector types