baton-github: add DefaultCapabilitiesBuilder#168
Conversation
|
|
||
| // DefaultCapabilitiesBuilder returns all resource types unconditionally so that | ||
| // the generated capabilities are always complete regardless of connector configuration. | ||
| func DefaultCapabilitiesBuilder() connectorbuilder.ConnectorBuilderV2 { |
There was a problem hiding this comment.
I am going to use it in baton-github-enterprise.
Bencheng21
force-pushed
the
ben/default_capabilities
branch
from
fe28f11 to
4dec331
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| type InvitationBuilderParams struct { | ||
| client *github.Client | ||
| orgCache *orgNameCache | ||
| orgs []string | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: InvitationBuilderParams is now exported, but all its fields (client, orgCache, orgs) are still unexported. External packages can reference the type but cannot construct a usable value. Either export the fields or keep the struct unexported.
| } | ||
|
|
||
| func orgBuilder(client, appClient *github.Client, orgCache *orgNameCache, orgs []string, syncSecrets bool) *orgResourceType { | ||
| func OrgBuilder(client, appClient *github.Client, orgCache *orgNameCache, orgs []string, syncSecrets bool) *orgResourceType { |
There was a problem hiding this comment.
🟡 Suggestion: All exported builder functions (e.g. OrgBuilder, TeamBuilder, UserBuilder, etc.) accept the unexported *orgNameCache parameter, and return unexported concrete types (e.g. *orgResourceType). External callers cannot construct orgNameCache (constructor newOrgNameCache is also unexported) and cannot name the return type. If these are meant to be usable outside the package, the shared dependencies and return types (or their constructors) need to be exported too. If they're only meant for internal use, consider keeping them unexported.
Connector PR Review: baton-github: add DefaultCapabilitiesBuilderBlocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0 Review SummaryThe new commits remove redundant Security IssuesNone found. Correctness IssuesNone found. SuggestionsNone. |
| if err != nil { | ||
| if isPermissionDenied(err) { | ||
| logger.Debug("baton-github: enterprise features (--enterprises) require a Personal Access Token. "+ | ||
| "GitHub App authentication cannot access the consumed-licenses API. "+ | ||
| "Either switch to PAT auth or remove the --enterprises flag.", | ||
| zap.String("enterprise", enterprise), | ||
| zap.Error(err)) | ||
| continue | ||
| } | ||
| return nil, nil, fmt.Errorf("baton-github: error listing enterprise consumed licenses for %s: %w", enterprise, err) |
There was a problem hiding this comment.
🟡 Suggestion: The removed isPermissionDenied guard provided graceful degradation for GitHub App auth users who configure --enterprises (the consumed-licenses API requires a PAT). Without it, the entire sync will now fail instead of skipping license data. Per R7, consider keeping the graceful degradation or gating this behind a config flag.
|
|
||
| func enterpriseRoleBuilder(client *github.Client, appClient *github.Client, customClient *customclient.Client, enterprises []string) *enterpriseRoleResourceType { | ||
| func EnterpriseRoleBuilder(client *github.Client, appClient *github.Client, customClient *customclient.Client, enterprises []string) *enterpriseRoleResourceType { | ||
| annos := annotations.Annotations(resourceTypeLicense.GetAnnotations()) |
There was a problem hiding this comment.
🟠 Bug: Copy-paste error — this reads annotations from resourceTypeLicense instead of resourceTypeEnterpriseRole. This gives the enterprise role resource type a V1Identifier with Id: "license" instead of "enterprise_role".
| annos := annotations.Annotations(resourceTypeLicense.GetAnnotations()) | |
| annos := annotations.Annotations(resourceTypeEnterpriseRole.GetAnnotations()) |
| if len(enterprises) == 0 { | ||
| annos.Append(&v2.SkipEntitlementsAndGrants{}) | ||
| } | ||
|
|
There was a problem hiding this comment.
oh I meant we should do this for users resource type. specifically if resource type license is not enabled
2 participants
No description provided.