Skip to content

Fixed Repo delete by name #6336

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.

Sign up for GitHub

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

Draft
DharunMR wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Fixed Repo delete by name #6336

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
49be524
Fixed Repo delete by name
DharunMR Apr 10, 2026
e61893c
added missing get repo testcases
DharunMR Apr 10, 2026
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
28 changes: 23 additions & 5 deletions internal/controlplane/handlers_repositories.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,11 @@ func (s *Server) RegisterRepository(
in *pb.RegisterRepositoryRequest,
) (*pb.RegisterRepositoryResponse, error) {
projectID := GetProjectID(ctx)
providerName := GetProviderName(ctx)

if in.GetContext() == nil || in.GetContext().GetProvider() == "" {
return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when registering a repository")
}
providerName := in.GetContext().GetProvider()

var fetchByProps *properties.Properties
var provider *db.Provider
Expand Down Expand Up @@ -137,10 +141,15 @@ func (s *Server) repoCreateInfoFromUpstreamEntityRef(
// This function will typically be called by the client to get a list of
// repositories that are registered present in the minder database
func (s *Server) ListRepositories(ctx context.Context,
_ *pb.ListRepositoriesRequest) (*pb.ListRepositoriesResponse, error) {
in *pb.ListRepositoriesRequest) (*pb.ListRepositoriesResponse, error) {

if in.GetContext() == nil || in.GetContext().GetProvider() == "" {
Copy link
Copy Markdown
Member

@evankanderson evankanderson Apr 11, 2026

Choose a reason for hiding this comment

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

GetProvider() already handles being called on a nil *Context -- using the proto getters will automatically handle empty / missing fields by returning the default (zero) value.

return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when listing repositories")
}
Copy link
Copy Markdown
Member

@evankanderson evankanderson Apr 11, 2026

Choose a reason for hiding this comment

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

This doesn't seem to crash or cause a problem today:

$ ./bin/minder repo list
 OWNER              │ NAME      │ PROVIDER                        │ UPSTREAM ID    
────────────────────┼───────────┼─────────────────────────────────┼────────────────
 a-random-sandbox   │ colorls   │ github-app-a-random-sandbox     │ 693510024      
                    ├───────────┤                                 ├────────────────
                    │ doitlive  │                                 │ 693507178

Copy link
Copy Markdown
Member

@evankanderson evankanderson Apr 11, 2026

Choose a reason for hiding this comment

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

With curl, in case you think there's some magic in the CLI:

$ curl -H "Authorization: Bearer $(jq -r .access_token ~/.config/minder/127.0.0.1_8090.json)" 'http://localhost:8080/api/v1/repositories/provider/' | jq '.results[] | .name'
...
"colorls"
"doitlive"


entityCtx := engcontext.EntityFromContext(ctx)
projectID := entityCtx.Project.ID
providerName := entityCtx.Provider.Name
providerName := in.GetContext().GetProvider()

logger.BusinessRecord(ctx).Provider = providerName
logger.BusinessRecord(ctx).Project = projectID
Expand Down Expand Up @@ -239,9 +248,13 @@ func (s *Server) GetRepositoryByName(ctx context.Context,
return nil, util.UserVisibleError(codes.InvalidArgument, "invalid repository name, needs to have the format: owner/name")
}

if in.GetContext() == nil || in.GetContext().GetProvider() == "" {
return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when getting a repository by name")
}

entityCtx := engcontext.EntityFromContext(ctx)
projectID := entityCtx.Project.ID
providerName := entityCtx.Provider.Name
providerName := in.GetContext().GetProvider()

// Use the service to fetch the repository
repo, err := s.repos.GetRepositoryByName(ctx, fragments[0], fragments[1], projectID, providerName)
Expand Down Expand Up @@ -298,7 +311,12 @@ func (s *Server) DeleteRepositoryByName(
}

projectID := GetProjectID(ctx)
providerName := GetProviderName(ctx)

if in.GetContext() == nil || in.GetContext().GetProvider() == "" {
return nil, util.UserVisibleError(codes.InvalidArgument, "provider name must be specified when deleting a repository by name")
}
Comment on lines +315 to +317
Copy link
Copy Markdown
Member

@evankanderson evankanderson Apr 11, 2026

Choose a reason for hiding this comment

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

This doesn't seem to crash today:

$ ./bin/minder repo delete -n a-random-sandbox/.github
Successfully deleted repo with name: a-random-sandbox/.github


providerName := in.GetContext().GetProvider()

err := s.repos.DeleteByName(ctx, fragments[0], fragments[1], projectID, providerName)
if errors.Is(err, sql.ErrNoRows) {
Expand Down
184 changes: 182 additions & 2 deletions internal/controlplane/handlers_repositories_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,16 @@ func TestServer_RegisterRepository(t *testing.T) {
RepoName string
RepoServiceSetup repoMockBuilder
ProviderFails bool
EmptyProvider bool
ExpectedError string
}{
{
Name: "Repo registration fails when provider is empty",
RepoOwner: repoOwner,
RepoName: repoName,
EmptyProvider: true,
ExpectedError: "provider name must be specified when registering a repository",
},
{
Name: "Repo creation fails when provider cannot be found",
RepoOwner: repoOwner,
Expand Down Expand Up @@ -136,11 +144,17 @@ func TestServer_RegisterRepository(t *testing.T) {
)

req := &pb.RegisterRepositoryRequest{
Context: &pb.Context{},
Repository: &pb.UpstreamRepositoryRef{
Owner: scenario.RepoOwner,
Name: scenario.RepoName,
},
}

if !scenario.EmptyProvider {
req.Context.Provider = ptr.Ptr(ghprovider.Github)
}

res, err := server.RegisterRepository(ctx, req)
if scenario.ExpectedError == "" {
expectation := &pb.RegisterRepositoryResponse{
Expand Down Expand Up @@ -169,9 +183,15 @@ func TestServer_ListRepositories(t *testing.T) {
RepoServiceSetup repoMockBuilder
ProviderSetup func(ctrl *gomock.Controller, mgr *mockmanager.MockProviderManager)
ProviderFails bool
EmptyProvider bool
ExpectedResults []*pb.Repository
ExpectedError string
}{
{
Name: "List repositories fails when provider is empty",
EmptyProvider: true,
ExpectedError: "provider name must be specified when listing repositories",
},
{
Name: "List repositories succeeds with multiple results",
RepoServiceSetup: rf.NewRepoService(
Expand Down Expand Up @@ -282,7 +302,14 @@ func TestServer_ListRepositories(t *testing.T) {
mgr,
)

req := &pb.ListRepositoriesRequest{}
req := &pb.ListRepositoriesRequest{
Context: &pb.Context{},
}

if !scenario.EmptyProvider {
req.Context.Provider = ptr.Ptr(ghprovider.Github)
}

res, err := server.ListRepositories(ctx, req)
if scenario.ExpectedError == "" {
require.NoError(t, err)
Expand Down Expand Up @@ -405,6 +432,146 @@ func TestServer_ListRemoteRepositoriesFromProvider(t *testing.T) {
}
}

func mockGetRepoByName(res *pb.Repository, err error) repoMockBuilder {
return func(ctrl *gomock.Controller) repoServiceMock {
mock := mockrepo.NewMockRepositoryService(ctrl)
mock.EXPECT().GetRepositoryByName(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(res, err).AnyTimes()
return mock
}
}

func mockGetRepoByID(res *pb.Repository, err error) repoMockBuilder {
return func(ctrl *gomock.Controller) repoServiceMock {
mock := mockrepo.NewMockRepositoryService(ctrl)
mock.EXPECT().GetRepositoryById(gomock.Any(), gomock.Any(), gomock.Any()).Return(res, err).AnyTimes()
return mock
}
}

func TestServer_GetRepository(t *testing.T) {
t.Parallel()

scenarios := []struct {
Name string
RepoName string
RepoID string
RepoServiceSetup repoMockBuilder
ProviderFails bool
EmptyProvider bool
ExpectedError string
}{
{
Name: "get by name fails when name is malformed",
RepoName: "I am not a repo name",
ExpectedError: "invalid repository name",
},
{
Name: "get by ID fails when ID is malformed",
RepoID: "I am not a UUID",
ExpectedError: "invalid repository ID",
},
{
Name: "get by name fails when provider is empty",
RepoName: repoOwnerAndName,
EmptyProvider: true,
ExpectedError: "provider name must be specified when getting a repository by name",
},
{
Name: "get by name fails when repo service returns error",
RepoName: repoOwnerAndName,
RepoServiceSetup: mockGetRepoByName(nil, errDefault),
ExpectedError: errDefault.Error(),
},
{
Name: "get by ID fails when repo service returns error",
RepoID: repoID,
RepoServiceSetup: mockGetRepoByID(nil, errDefault),
ExpectedError: "cannot read repository",
},
{
Name: "get by name fails when repo is not found",
RepoName: repoOwnerAndName,
RepoServiceSetup: mockGetRepoByName(nil, sql.ErrNoRows),
ExpectedError: "repository not found",
},
{
Name: "get by ID fails when repo is not found",
RepoID: repoID,
RepoServiceSetup: mockGetRepoByID(nil, sql.ErrNoRows),
ExpectedError: "repository not found",
},
{
Name: "get by name succeeds",
RepoName: repoOwnerAndName,
RepoServiceSetup: mockGetRepoByName(&existingRepo, nil),
},
{
Name: "get by ID succeeds",
RepoID: repoID,
RepoServiceSetup: mockGetRepoByID(&existingRepo, nil),
},
}

for _, scenario := range scenarios {
t.Run(scenario.Name, func(t *testing.T) {
t.Parallel()
ctrl := gomock.NewController(t)
defer ctrl.Finish()
ctx := engcontext.WithEntityContext(context.Background(), &engcontext.EntityContext{
Provider: engcontext.Provider{Name: ghprovider.Github},
Project: engcontext.Project{ID: projectID},
})

server := createServer(
ctrl,
scenario.RepoServiceSetup,
scenario.ProviderFails,
nil,
)

var result *pb.Repository
var resultError error

if scenario.RepoName != "" {
req := &pb.GetRepositoryByNameRequest{
Name: scenario.RepoName,
Context: &pb.Context{},
}

if !scenario.EmptyProvider {
req.Context.Provider = ptr.Ptr(ghprovider.Github)
}

res, err := server.GetRepositoryByName(ctx, req)
if res != nil {
result = res.Repository
}
resultError = err
} else {
req := &pb.GetRepositoryByIdRequest{
RepositoryId: scenario.RepoID,
}
res, err := server.GetRepositoryById(ctx, req)
if res != nil {
result = res.Repository
}
resultError = err
}

if scenario.ExpectedError == "" {
require.NoError(t, resultError)
require.NotNil(t, result)
if scenario.RepoName != "" {
require.Equal(t, scenario.RepoName, fmt.Sprintf("%s/%s", result.Owner, result.Name))
}
} else {
require.Nil(t, result)
require.ErrorContains(t, resultError, scenario.ExpectedError)
}
})
}
}

// lump both deletion endpoints together since they are so similar
func TestServer_DeleteRepository(t *testing.T) {
t.Parallel()
Expand All @@ -415,6 +582,7 @@ func TestServer_DeleteRepository(t *testing.T) {
RepoID string
RepoServiceSetup repoMockBuilder
ProviderFails bool
EmptyProvider bool
ExpectedError string
}{
{
Expand All @@ -427,6 +595,12 @@ func TestServer_DeleteRepository(t *testing.T) {
RepoID: "I am not a UUID",
ExpectedError: "invalid repository ID",
},
{
Name: "delete by name fails when provider is empty",
RepoName: repoOwnerAndName,
EmptyProvider: true,
ExpectedError: "provider name must be specified when deleting a repository by name",
},
{
Name: "deletion fails when repo service returns error",
RepoName: repoOwnerAndName,
Expand Down Expand Up @@ -485,8 +659,14 @@ func TestServer_DeleteRepository(t *testing.T) {
var expectation string
if scenario.RepoName != "" {
req := &pb.DeleteRepositoryByNameRequest{
Name: scenario.RepoName,
Name: scenario.RepoName,
Context: &pb.Context{},
}

if !scenario.EmptyProvider {
req.Context.Provider = ptr.Ptr(ghprovider.Github)
}

res, err := server.DeleteRepositoryByName(ctx, req)
if res != nil {
result = res.Name
Expand Down
Loading