Conversation
WalkthroughGo toolchain and numerous dependencies updated; a new SCIM configuration schema, validation, and generated Batonconfig accessors were added; and a unit test expectation for service providers was adjusted. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
gontzess
changed the title
gontzess
force-pushed
the
task-sdk-upgrade-batch-10
branch
2 times, most recently
from
588d19a to
082b2ea
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@pkg/batonconfig/config.go`:
- Line 85: The error string returned in pkg/batonconfig/config.go references a
non-existent "token" field; update the message in the failing-return (the
fmt.Errorf call) to only mention the actual configurable credentials used by
this connector (for example "api-key", "username and password", and
"scim-client-id and scim-client-secret"), or otherwise replace "token" with the
correct field name used in this package, so the error accurately reflects the
available fields.
- Around line 14-17: Remove the unused legacy ScimConfigField declaration and
its inclusion in the Config struct (replace usages to use ScimConfigFileField
where appropriate) — ScimConfigField (name "scim-config") is redundant because
the code reads "scim-config-file" via ScimConfigFileField; also update the
erroneous error message that mentions "token" to reference the correct config
field name ("scim-config-file" or ScimConfigFileField) in the validation/error
routine so logs reflect the actual missing/invalid config field (check usages in
cmd/baton-scim/main.go and pkg/scim/client.go to ensure they reference
ScimConfigFileField).
In `@pkg/config/scim_config_test.go`:
- Line 15: The test uses require.Equal with the arguments reversed; update the
assertion in scim_config_test.go to follow testify's convention by calling
require.Equal(t, 4, len(providers)) or, even clearer, replace it with
require.Len(t, providers, 4); locate the current require.Equal(t,
len(providers), 4) (or similar) and swap the expected and actual or use
require.Len to fix it.
🧹 Nitpick comments (1)
pkg/batonconfig/config.go (1)
59-61: Consider usingFieldRelationshipsfor declarative validation.
FieldRelationshipsis defined but empty. The baton-sdk supports declarative field relationships (e.g., mutual exclusivity, required together) that could replace some of the imperative validation inValidateConfig. This would provide better integration with SDK tooling and clearer configuration requirements.
|
|
||
| require.NoError(t, err) | ||
| require.Equal(t, len(providers), 5) | ||
| require.Equal(t, len(providers), 4) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the test file around line 15
echo "=== Content of scim_config_test.go around line 15 ==="
head -30 pkg/config/scim_config_test.go | cat -n
# Find the getServiceProviders function
echo -e "\n=== Search for getServiceProviders function ==="
rg -n "getServiceProviders|func.*ServiceProviders" --type go | head -20Repository: ConductorOne/baton-scim
Length of output: 1315
🏁 Script executed:
#!/bin/bash
# Look for provider definitions and initialization
echo "=== Search for provider definitions ==="
rg -n "ServiceProvider|providers\s*=" --type go -B 2 -A 2 | head -50
# Check the scim_config.go file for provider initialization
echo -e "\n=== Check scim_config.go ==="
head -100 pkg/config/scim_config.go | cat -nRepository: ConductorOne/baton-scim
Length of output: 9199
🌐 Web query:
testify require.Equal argument order convention
💡 Result:
In testify v1.x, the convention (and actual function signature) for require.Equal / assert.Equal is:
require.Equal(t, expected, actual)This matches the published signature Equal(t, expected, actual, ...). [1][2][3]
Note: there’s an ongoing testify v2.0.0 API discussion proposing switching to actual, expected (i.e., “got, want”) for new/updated assertions, but that’s a guideline proposal—not how v1 works today. [2]
🏁 Script executed:
#!/bin/bash
# Get the getServiceProviders function implementation
echo "=== getServiceProviders function implementation ==="
sed -n '212,250p' pkg/config/scim_config.go | cat -n
# List the service_providers directory
echo -e "\n=== Service providers directory contents ==="
find pkg/config/service_providers -type f | sortRepository: ConductorOne/baton-scim
Length of output: 1036
🏁 Script executed:
#!/bin/bash
# Get more context from getServiceProviders function
echo "=== Full getServiceProviders function ==="
sed -n '212,230p' pkg/config/scim_config.go | cat -nRepository: ConductorOne/baton-scim
Length of output: 820
Fix require.Equal argument order to follow testify convention.
The testify convention is require.Equal(t, expected, actual). The arguments are currently reversed—the assertion should be require.Equal(t, 4, len(providers)).
Suggested fix
- require.Equal(t, len(providers), 4)
+ require.Equal(t, 4, len(providers))Alternatively, use require.Len(t, providers, 4) for a clearer assertion message.
The provider count of 4 is correct—the service_providers directory currently contains miro.yaml, postman.yaml, slack.yaml, and zoom.yaml.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| require.Equal(t, len(providers), 4) | |
| require.Equal(t, 4, len(providers)) |
🤖 Prompt for AI Agents
In `@pkg/config/scim_config_test.go` at line 15, The test uses require.Equal with
the arguments reversed; update the assertion in scim_config_test.go to follow
testify's convention by calling require.Equal(t, 4, len(providers)) or, even
clearer, replace it with require.Len(t, providers, 4); locate the current
require.Equal(t, len(providers), 4) (or similar) and swap the expected and
actual or use require.Len to fix it.
gontzess
force-pushed
the
task-sdk-upgrade-batch-10
branch
from
082b2ea to
fca3add
Compare
gontzess
changed the title
1 participant
Summary
WithDefaultCapabilitiesConnectorBuilderoption for configless capabilitiesBuild Status
go build ./...passes locallyTest plan