-
Notifications
You must be signed in to change notification settings - Fork 109
feat(entities): Implement ENTITY_ORGANIZATION for Github Providers #6356
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
56cbde2
26ed1fc
4000a99
3b5eb14
cae8441
bda676f
34c66c2
ab5e202
9a4b2be
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| -- SPDX-FileCopyrightText: Copyright 2026 The Minder Authors | ||
| -- SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| -- Postgres doesn't easily drop enum values, down migration is a no-op |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| -- SPDX-FileCopyrightText: Copyright 2026 The Minder Authors | ||
| -- SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| ALTER TYPE entities ADD VALUE 'organization'; |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -427,7 +427,8 @@ func (s *Server) getRuleEvalStatus( | |
| repoPath = fmt.Sprintf("%s/%s", prRepoOwner, prRepoName) | ||
| } | ||
| case db.EntitiesBuildEnvironment, db.EntitiesRelease, db.EntitiesPipelineRun, | ||
| db.EntitiesTaskRun, db.EntitiesBuild: | ||
| db.EntitiesTaskRun, db.EntitiesBuild, db.EntitiesOrganization: | ||
|
Comment on lines
429
to
+430
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is probably okay for now, but it seems like alerts will have incorrect URLs (and log errors) until we fix it. It can be a good idea to file an issue for this if we merge this PR, to track the need to update it later. |
||
| // TODO: Alert URLs for organizations are incorrect | ||
| zerolog.Ctx(ctx).Warn().Msgf("attempting to set alerts for unsupported entity type: %v", dbRuleEvalStat.EntityType) | ||
| default: | ||
| zerolog.Ctx(ctx).Error().Msgf("unknown entity type: %v", dbRuleEvalStat.EntityType) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -134,11 +134,14 @@ func (s *Server) claimGitHubInstalls(ctx context.Context, qtx db.ExtendQuerier) | |
|
|
||
| for _, i := range installs { | ||
| // TODO: if we can get an GitHub auth token for the user, we can do the rest with CreateGitHubAppWithoutInvitation | ||
| proj, err := s.ghProviders.CreateGitHubAppWithoutInvitation(ctx, qtx, userID, i.AppInstallationID) | ||
| proj, dbProv, err := s.ghProviders.CreateGitHubAppWithoutInvitation(ctx, qtx, userID, i.AppInstallationID) | ||
| if err != nil { | ||
| zerolog.Ctx(ctx).Error().Err(err).Int64("org_id", i.OrganizationID).Msg("failed to create GitHub app at first login") | ||
| continue | ||
| } | ||
| if dbProv != nil { | ||
| s.publishOrganizationEntityEvent(ctx, dbProv.Provider.ID, proj.ID, dbProv.InstallationOwner) | ||
| } | ||
|
Comment on lines
+142
to
+144
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, let's try to centralize this pattern in the GitHub provider. |
||
| if proj != nil { | ||
| userProjects = append(userProjects, proj) | ||
| } | ||
|
|
||
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -255,7 +255,8 @@ func (e *EEA) buildEntityWrapper( | |
| case db.EntitiesPullRequest: | ||
| return e.buildPullRequestInfoWrapper(ctx, entityID, projID) | ||
| case db.EntitiesBuildEnvironment, db.EntitiesRelease, | ||
| db.EntitiesPipelineRun, db.EntitiesTaskRun, db.EntitiesBuild: | ||
| db.EntitiesPipelineRun, db.EntitiesTaskRun, db.EntitiesBuild, db.EntitiesOrganization: | ||
| // TODO: Support evaluate policy on organizations | ||
| return nil, fmt.Errorf("entity type %q not yet supported", entity) | ||
|
Comment on lines
+258
to
260
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think leaving this this way will prevent us from evaluating policy on organizations (but I'd need to test to be sure). Definitely something to file a follow-up issue on if we leave like this. |
||
| default: | ||
| return nil, fmt.Errorf("unknown entity type: %q", entity) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the only time the return value from
CreateGitHubAppProvideris used is in a unit test, it should be easy to change that to return a provider facet or some other struct which can be used to resolve any provider properties we need. (Currently, it seems like mostly the org name, but possibly other data in the future.)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than returning a bunch of state which needs to be passed along to
publishOrganizationEntityEvent, let's move that call into theghProvidersservice, rather than having it here and inhandlers_user.go.