Disable custom roles by default - #62
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| field.WithDisplayName("Disable custom roles sync"), | ||
| field.WithDescription("Whether to disable custom roles sync or not"), | ||
| field.WithDefaultValue(false), | ||
| field.WithDefaultValue(true), |
There was a problem hiding this comment.
🟠 Bug: This is an ungated default-on breaking change for existing installs. Any deployment that never explicitly set disable-custom-roles-sync flips to true on upgrade, and the next sync will emit only base roles from role.go:99-109, return no folder→role grants (folder.go:112-114), and skip collaborator→custom-role grants (collaborator.go:266-268) — C1 reads those missing resources/grants as deletions and access revocations. Note this also propagates to the platform: boolField currently omits defaultValue in config_schema.json only because false is the proto3 zero value, so after merge the auto-regenerated schema will publish defaultValue: true to the GUI too. Per the repo's breaking-change gate, this needs to stay opt-in (or at minimum be called out in the PR description with a migration note and paired docs/connector.mdx + README.md updates telling legacy-roles workspaces to set --disable-custom-roles-sync=false).
There was a problem hiding this comment.
I have confirmed that all connectors have disable-custom-roles-sync explicitly set in their configurations.
Connector PR Review: Disable custom roles by defaultBlocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff ( Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agentsNote: the machine-readable |
Addresses PR review: README still described the flag as an off-by-default opt-in; add a test asserting the resolved default so a future flip fails CI.
| > **Note:** The **Collaborator roles** section (including "List non-system roles") only appears in the API client permissions UI if your workspace still uses the legacy roles model. If this section is not visible, your workspace has migrated to the new RBAC v2 model (Environment roles + Project roles). In that case, legacy custom roles are not accessible via the API and you should either: | ||
| > - Migrate your legacy custom roles to the new model using the [Role migration API](https://docs.workato.com/workato-api/role-migration.html), or | ||
| > - Set `--disable-custom-roles-sync=true` to skip legacy custom role sync and continue with base and environment roles only. | ||
| > **Note:** The **Collaborator roles** section (including "List non-system roles") only appears in the API client permissions UI if your workspace still uses the legacy roles model. If this section is not visible, your workspace has migrated to the new RBAC v2 model (Environment roles + Project roles). In that case, legacy custom roles are not accessible via the API — and legacy custom role sync is already skipped by default (`--disable-custom-roles-sync` defaults to `true`), so no action is needed. |
There was a problem hiding this comment.
🟡 Suggestion: With the default now true, every GetRoles call site is behind !disableCustomRolesSync (role.go:62, role.go:189 via ensureRolesCache, folder.go:112 early-return, collaborator.go:247), so GET /api/roles is never called on a default install. The "Required Client Role permissions" table above (line 26, and the mirror at docs/connector.mdx:84) still lists List non-system roles as required — it should be marked conditional, i.e. required only when --disable-custom-roles-sync=false. The footnotes explain UI visibility, not that the permission is now optional.
| func TestDisableCustomRolesSyncDefault(t *testing.T) { | ||
| defaultValue, err := field.GetDefaultValue[bool](DisableCustomRolesSync) | ||
| require.NoError(t, err) | ||
| require.NotNil(t, defaultValue) | ||
| require.True(t, *defaultValue, "disable-custom-roles-sync should default to true") | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: field.GetDefaultValue[bool] just type-asserts SchemaField.DefaultValue (vendor/.../pkg/field/fields.go:185), so this asserts the literal field.WithDefaultValue(true) two lines away in config.go:46 rather than the resolved default the comment claims. It would not catch a regression in the resolution layer (viper/pflag binding, schema export, conf.gen.go). Consider instead adding a test.TestCase to the currently-empty testCases slice at line 18 that resolves config with the flag unset and asserts the effective DisableCustomRolesSync value — that exercises observable behavior and covers both this and the prior review's "disabled-path" gap.
| "github.com/conductorone/baton-sdk/pkg/field" | ||
| "github.com/conductorone/baton-sdk/pkg/test" | ||
| "github.com/spf13/viper" | ||
| "github.com/stretchr/testify/require" |
There was a problem hiding this comment.
🟡 Suggestion: go.mod:107 still records github.com/stretchr/testify v1.11.1 // indirect, but this is now a direct import of the main module. The build is fine (vendor/modules.txt:627 already marks the module ## explicit and vendors testify/require), but go mod tidy would drop the // indirect marker, so the manifest no longer matches the code and a tidy-diff check in the shared verify workflow would flag it. Worth running make add-dep and committing the go.mod change.
I addressed the comment
2 participants
Description
Disables legacy custom roles sync by default, since Workato is retiring legacy roles at the end of August 2026. Regenerates
config_schema.jsonand updatesdocs/connector.mdxandREADME.mdto match the new default. Adds a test pinning the resolved default so a future accidental flip fails CI.Breaking-change note: flipping the default affects any install that never explicitly set
disable-custom-roles-sync. I've manually confirmed that all existing connector configurations already have this setting present explicitly, so this default change does not silently alter behavior for any current install.Useful links: