Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion docs/connector.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ Use this integration if your organization accesses GitHub at `github.com`. If yo
| GitHub Apps (NHI) | <Icon icon="square-check" iconType="solid" color="#c937ae"/> | |
| Secrets - API keys | <Icon icon="square-check" iconType="solid" color="#c937ae"/> | |

The GitHub connector supports [automatic account provisioning and deprovisioning](/product/admin/account-provisioning). New accounts will send an invitation to the account owner; if an invitation is pending, the account status will be shown as **Unspecified**.
The GitHub connector supports [automatic account provisioning and deprovisioning](/product/admin/account-provisioning). New accounts will send an invitation to the account owner; while an invitation is pending, the account status will be shown as **Pending**. Expired invitations also report **Pending**, distinguished by `invitation_expired` in the status details.

Repository permissions that are inherited through team membership are labeled as such on the relevant entitlement's **Grants** tab in the C1 web app.

Expand Down
2 changes: 1 addition & 1 deletion pkg/connector/connector.go
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ var (
resourceTypeInvitation = &v2.ResourceType{
Id: "invitation",
DisplayName: "Invitation",
// Invitations emit TRAIT_USER with UserTrait_Status_STATUS_UNSPECIFIED.
// Invitations emit TRAIT_USER with STATUS_PENDING.
// Accepted members from user.go emit STATUS_ENABLED.
Traits: []v2.ResourceType_Trait{
v2.ResourceType_TRAIT_USER,
Expand Down
14 changes: 4 additions & 10 deletions pkg/connector/invitation.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,19 +64,13 @@ func invitationToUserResource(invitation *github.Invitation, status string) (*v2
invitation.GetID(),
[]resourceSdk.UserTraitOption{
resourceSdk.WithEmail(invitation.GetEmail(), true),
// An invitation is a pending/expired user that must not be
// reported as enabled. WithResourceStatus cannot express this:
// NewUserTrait force-defaults an unset trait status to ENABLED, so
// migrating this line would flip the emitted status from
// UNSPECIFIED to ENABLED. Keep the deprecated trait option (which
// also mirrors UNSPECIFIED to the resource level) to preserve the
// exact status semantics.
//nolint:staticcheck // deliberate: WithResourceStatus would force the trait status to ENABLED; UNSPECIFIED must be preserved for invitations.
resourceSdk.WithStatus(v2.UserTrait_Status_STATUS_UNSPECIFIED),
resourceSdk.WithUserLogin(login),
},
// profile has moved from UserTrait to a Resource-level attribute.
// profile and status have moved from UserTrait to Resource-level
// attributes. Expired invitations stay PENDING - they are still not a
// usable account - and carry the distinction in the status details.
resourceSdk.WithResourceProfile(profile),
resourceSdk.WithResourceStatus(v2.Status_RESOURCE_STATUS_PENDING, status),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Suggestion: docs/connector.mdx:26 still says "if an invitation is pending, the account status will be shown as Unspecified", which this change makes wrong. Update it to Pending in this PR so the docs don't lag the emitted status.

Separately, both status fields carry defined_only enum validation, so a reader still on the pre-PENDING proto descriptor would reject enum value 4 rather than fall back to unspecified. The PR description covers the c1-first rollout ordering, but nothing in the connector enforces it — worth confirming that ordering is tracked before this leaves draft, since there is no config gate to fall back to.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Docs fixed in 382a57f — connector.mdx now says Pending, with the expired-invitations nuance (also Pending, distinguished by invitation_expired in status details).

On the defined_only ordering concern: the situation changed since this review ran. c1 PR ductone/c1#24091 was narrowed to treat only explicit STATUS_PENDING as pending (UNSPECIFIED turned out to be emitted for real accounts by other connectors). We also verified c1's c1z ingestion runs no enum validation on synced resources — a pre-bump c1 decodes value 4 and maps it to UNSPECIFIED via the existing default arm, i.e. today's behavior exactly. So no ordering is unsafe and no config gate is needed; the feature simply stays inert until c1 ships its vendored-SDK bump mapping PENDING. The PR description's Rollout section is updated with this.

)
if err != nil {
return nil, err
Expand Down
29 changes: 29 additions & 0 deletions pkg/connector/invitation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,7 @@ func TestInvitationListPagination(t *testing.T) {
pendingCreated1.Add(invitationLifetime).UTC().Format(time.RFC3339),
aliceProfile["invitation_expires_at"],
)
requireInvitationPending(t, byID["1001"], invitationStatusPendingAcceptance)

// Expired resources carry status=expired and expires_at = failed_at.
daveProfile := invitationProfile(t, byID["2001"])
Expand All @@ -207,6 +208,9 @@ func TestInvitationListPagination(t *testing.T) {
expiredFailedAt1.UTC().Format(time.RFC3339),
daveProfile["invitation_expires_at"],
)
// An expired invitation is still not a usable account, so it stays
// PENDING at both levels; only the details distinguish it.
requireInvitationPending(t, byID["2001"], invitationStatusExpired)
})

t.Run("pending 404 falls through to failed", func(t *testing.T) {
Expand All @@ -222,6 +226,7 @@ func TestInvitationListPagination(t *testing.T) {
require.Equal(t, "2001", got[0].Id.Resource)
require.Equal(t, invitationStatusExpired,
invitationProfile(t, got[0])["invitation_status"])
requireInvitationPending(t, got[0], invitationStatusExpired)
})

t.Run("failed 404 terminates cleanly", func(t *testing.T) {
Expand All @@ -236,6 +241,7 @@ func TestInvitationListPagination(t *testing.T) {
require.Len(t, got, 2)
require.Equal(t, invitationStatusPendingAcceptance,
invitationProfile(t, got[0])["invitation_status"])
requireInvitationPending(t, got[0], invitationStatusPendingAcceptance)
})

t.Run("both endpoints empty terminates without API errors", func(t *testing.T) {
Expand All @@ -262,3 +268,26 @@ func invitationProfile(t *testing.T, r *v2.Resource) map[string]any {
require.NotNil(t, profile)
return profile.AsMap()
}

// requireInvitationPending asserts that an invitation resource reports PENDING
// at both the resource level and the (deprecated) user-trait level, with
// details naming which flavor of pending it is.
func requireInvitationPending(t *testing.T, r *v2.Resource, wantDetails string) {
t.Helper()

require.Equal(t, v2.Status_RESOURCE_STATUS_PENDING, r.GetStatus().GetStatus())
require.Equal(t, wantDetails, r.GetStatus().GetDetails())

// Consumers resolve status resource-first via GetStatus; the trait-level
// status is left to NewUserTrait's ENABLED force-default and must not be
// read directly.
resolved := resourceSdk.GetStatus(r)
require.Equal(t, v2.Status_RESOURCE_STATUS_PENDING, resolved.GetStatus())
require.Equal(t, wantDetails, resolved.GetDetails())

ut, err := resourceSdk.GetUserTrait(r)
require.NoError(t, err)
require.NotNil(t, ut)
//nolint:staticcheck // pins the known force-default artifact so a change to it is caught.
require.Equal(t, v2.UserTrait_Status_STATUS_ENABLED, ut.GetStatus().GetStatus())
}
Loading