Add delete provider command #2922
Conversation
eleftherias
force-pushed
the
delete-provider
branch
from
d695be8 to
267321b
Compare
eleftherias
force-pushed
the
delete-provider
branch
from
267321b to
13ff368
Compare
eleftherias
force-pushed
the
delete-provider
branch
from
13ff368 to
6799427
Compare
| if p.Implements(db.ProviderTypeGithub) && | ||
| providers.GetCredentialStateForProvider(ctx, *provider, s.store, s.cryptoEngine, &s.cfg.Provider) == | ||
| provinfv1.CredentialStateSet { | ||
| client, err := p.GetGitHub() | ||
| if err != nil { | ||
| return status.Errorf(codes.Internal, "error creating github provider: %v", err) | ||
| } | ||
|
|
||
| // Delete all repositories associated with the provider and remove the webhooks | ||
| err = s.repos.DeleteRepositoriesByProvider(ctx, client, provider.Name, projectID) | ||
| if err != nil { | ||
| return status.Errorf(codes.Internal, "error deleting repositories: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
This feels like it should be a part of the DeleteProvider implementation, rather than in ControlPlane. e.g. GitHub providers should understand that they need to clean up webhooks, but other types of providers might have other types of cleanup they need to do.
(e.g. we talked about DeleteProvider for GHA un-installing the app from the org.)
There was a problem hiding this comment.
I had written it like that initially, but then providerService needed to use the repositoryService which seemed wrong. We could also call the store directly to delete the repositories, but then there will be duplicate logic in the services.
There was a problem hiding this comment.
I think it's totally reasonable for different services to call eachother - some domain concepts are at a higher level of abstraction than others, and thus delegate to the services for the lower-level domain concepts.
There was a problem hiding this comment.
It seems there were some recent changes that also now create an import cycle when I try that:
package github.com/stacklok/minder/internal/providers
imports github.com/stacklok/minder/internal/repositories/github
imports github.com/stacklok/minder/internal/logger
imports github.com/stacklok/minder/internal/engine/actions/alert
imports github.com/stacklok/minder/internal/engine/actions/alert/security_advisory
imports github.com/stacklok/minder/internal/providers: import cycle not allowed
There was a problem hiding this comment.
I feel like we can resolve the cycle, but it's not something I want to spend time too much time on now. Can we live with this logic in the controlplane for now and do a followup PR to refactor?
There was a problem hiding this comment.
Looking at the import cycle, I'd probably break the logger --> actions/alert import path, but happy to do that as a follow-up.
| privateKey, err := p.config.GitHubApp.GetPrivateKey() | ||
| if err != nil { | ||
| return fmt.Errorf("error getting GitHub App private key: %w", err) | ||
| } | ||
| jwt, err := credentials.CreateGitHubAppJWT(p.config.GitHubApp.AppID, privateKey) | ||
| if err != nil { | ||
| return fmt.Errorf("error creating GitHub App JWT: %w", err) | ||
| } |
There was a problem hiding this comment.
These lines are shared with getInstallationOwner on 430 -- it feels like this should be a method.
getInstallationTokenCredential has a slightly different path to the same outcome, but might be the right tool for the job as it covers 417-436, with the chance of returning an EmptyCredential.
There was a problem hiding this comment.
(This is a small nit, and we can fix it later)
eleftherias
force-pushed
the
delete-provider
branch
from
6799427 to
c55cfa6
Compare
| if p.Implements(db.ProviderTypeGithub) && | ||
| providers.GetCredentialStateForProvider(ctx, *provider, s.store, s.cryptoEngine, &s.cfg.Provider) == | ||
| provinfv1.CredentialStateSet { | ||
| client, err := p.GetGitHub() | ||
| if err != nil { | ||
| return status.Errorf(codes.Internal, "error creating github provider: %v", err) | ||
| } | ||
|
|
||
| // Delete all repositories associated with the provider and remove the webhooks | ||
| err = s.repos.DeleteRepositoriesByProvider(ctx, client, provider.Name, projectID) | ||
| if err != nil { | ||
| return status.Errorf(codes.Internal, "error deleting repositories: %v", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
Looking at the import cycle, I'd probably break the logger --> actions/alert import path, but happy to do that as a follow-up.
eleftherias
force-pushed
the
delete-provider
branch
from
c55cfa6 to
ccbf3a2
Compare
Summary
This adds the
minder provider deletecommand, with the option of passing a provider ID or name.The command will delete the provider from the DB, delete all associated repos, remove the repo webhooks and uninstall the GitHub App from the GitHub org, if it is a GitHub App provider.
Fixes #2897
Change Type
Mark the type of change your PR introduces:
Testing
Outline how the changes were tested, including steps to reproduce and any relevant configurations.
Attach screenshots if helpful.
Review Checklist: