Skip to content

fix: remove enterpriseLicensesAvailable gate from ResourceSyncers - #138

Merged
madison-c-evans merged 1 commit into
mainfrom
cxh-1180/fix-resourcesyncers-gate
Apr 7, 2026
Merged

fix: remove enterpriseLicensesAvailable gate from ResourceSyncers#138
madison-c-evans merged 1 commit into
mainfrom
cxh-1180/fix-resourcesyncers-gate

Conversation

@madison-c-evans

Copy link
Copy Markdown
Contributor

Summary

  • Remove enterpriseLicensesAvailable gate from ResourceSyncers() — it was always false because the SDK calls ResourceSyncers() during newBuilder() initialization, before Validate() runs
  • Enterprises are now always passed through to builders; graceful degradation happens at sync time in fetchAndStoreEnterpriseSAML

What went wrong

During end-to-end validation on the sandbox, SAML emails were missing. Debug logs showed checkOrgSAML hitting samlStateDisabled ("no SAML identity provider found for org, disabling SAML enrichment") because len(u.enterprises) == 0 — the enterprises slice was empty despite being configured. Root cause: ResourceSyncers() was gating on a flag set in Validate(), but the SDK calls ResourceSyncers() first.

Test plan

  • Verified locally with go mod replace pointing baton-github-enterprise at this fix
  • SAML emails now sync correctly (3 users with enterprise SAML emails populated)
  • go build and go test pass

🤖 Generated with Claude Code

The SDK calls ResourceSyncers() during connector initialization
(newBuilder), which runs BEFORE Validate(). The
enterpriseLicensesAvailable flag was set in Validate, so it was always
false when ResourceSyncers ran — causing enterprises to never be
passed to userBuilder or enterpriseRoleBuilder.

Found during end-to-end validation: SAML emails were missing because
checkOrgSAML saw an empty enterprises slice and defaulted to
samlStateDisabled.

Fix: always pass gh.enterprises through in ResourceSyncers. Graceful
degradation for inaccessible APIs is already handled at sync time by
fetchAndStoreEnterpriseSAML (which warns and falls back to
samlStateDisabled). The Validate probe is kept as a diagnostic debug
log only.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@madison-c-evans
madison-c-evans requested a review from a team April 7, 2026 20:23
@linear

linear Bot commented Apr 7, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Apr 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @madison-c-evans's task in 56s —— View job


PR Review: fix: remove enterpriseLicensesAvailable gate from ResourceSyncers

  • Gather context and read changed files
  • Analyze the fix and its correctness
  • Post review feedback

Verdict: Looks good

This is a clean, well-scoped fix for a real ordering bug. The analysis is correct:

The bug: ResourceSyncers() is called by the SDK during newBuilder() initialization, which runs before Validate(). The enterpriseLicensesAvailable flag was only set in Validate(), so it was always false when ResourceSyncers() ran. This meant enterprises was always passed as an empty slice, so enterprise role sync and SAML enrichment never activated.

The fix: Remove the dead gate entirely and pass gh.enterprises directly. This is the right approach because:

  1. Graceful degradation already exists at sync timefetchAndStoreEnterpriseSAML (pkg/connector/user.go:154-162) catches API errors, logs them as debug, and falls back to samlStateDisabled. So the consumed-licenses check in Validate() was redundant protection.

  2. The Validate() probe is kept as a warning — the debug log at lines 215-216 and 237-239 now warns that "enterprise SAML email enrichment and enterprise role sync may fail at sync time" rather than claiming to gate anything. This gives operators a heads-up without incorrectly suppressing functionality.

  3. No leftover references to the removed fieldenterpriseLicensesAvailable is fully excised (struct field, both Validate branches, and the ResourceSyncers conditional).

One minor observation: The Validate() probe still makes an API call to ListEnterpriseConsumedLicenses purely for a debug log. This is fine as a diagnostic signal, but if you ever want to simplify further, it could be removed since the real error handling happens in fetchAndStoreEnterpriseSAML. Not suggesting a change here — just noting it.

No issues found. Ship it.

@madison-c-evans

Copy link
Copy Markdown
Contributor Author

Email loading issue still seen when I tried validation. The SDK calls ResourceSyncers() during connector initialization, before Validate() ever runs. The enterpriseLicensesAvailable flag was set in Validate(), so it was always false when ResourceSyncers() checked it, meaning enterprises were never passed to the user builder. That's why checkOrgSAML saw an empty enterprises list and disabled SAML enrichment. The fix: skip the gate entirely in ResourceSyncers() and always pass enterprises through - graceful degradation already happens at sync time in fetchAndStoreEnterpriseSAML

@madison-c-evans
madison-c-evans merged commit d0710af into main Apr 7, 2026
8 checks passed
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