Skip to content
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

fix: Allow to delete repos with invalid urls (#20921) #20975

Merged
merged 2 commits into from
Dec 2, 2024
Merged

fix: Allow to delete repos with invalid urls (#20921) #20975

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6ccd0fc
fix: Allow to delete repos with invalid urls (#20921)
andrii-korotkov-verkada Nov 27, 2024
c9018bc
Don't modify the original NormalizeGitURL
andrii-korotkov-verkada Dec 2, 2024
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
33 changes: 33 additions & 0 deletions server/repository/repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (

"github.com/argoproj/argo-cd/v2/common"
"github.com/argoproj/argo-cd/v2/pkg/apiclient/repository"
repositorypkg "github.com/argoproj/argo-cd/v2/pkg/apiclient/repository"
appsv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
fakeapps "github.com/argoproj/argo-cd/v2/pkg/client/clientset/versioned/fake"
appinformer "github.com/argoproj/argo-cd/v2/pkg/client/informers/externalversions"
Expand Down Expand Up @@ -1119,3 +1120,35 @@ func TestGetRepository(t *testing.T) {
})
}
}

func TestDeleteRepository(t *testing.T) {
repositories := map[string]string{
"valid": "https://bitbucket.org/workspace/repo.git",
// Check a wrongly formatter repo as well, see https://github.com/argoproj/argo-cd/issues/20921
"invalid": "git clone https://bitbucket.org/workspace/repo.git",
}

kubeclientset := fake.NewSimpleClientset(&argocdCM, &argocdSecret)
settingsMgr := settings.NewSettingsManager(context.Background(), kubeclientset, testNamespace)

for name, repo := range repositories {
t.Run(name, func(t *testing.T) {
repoServerClient := mocks.RepoServerServiceClient{}
repoServerClient.On("TestRepository", mock.Anything, mock.Anything).Return(&apiclient.TestRepositoryResponse{}, nil)

repoServerClientset := mocks.Clientset{RepoServerServiceClient: &repoServerClient}
enforcer := newEnforcer(kubeclientset)

db := &dbmocks.ArgoDB{}
db.On("DeleteRepository", context.TODO(), repo, "default").Return(nil)
db.On("ListRepositories", context.TODO()).Return([]*appsv1.Repository{{Repo: repo, Project: "default"}}, nil)
db.On("GetRepository", context.TODO(), repo, "default").Return(&appsv1.Repository{Repo: repo, Project: "default"}, nil)
appLister, projLister := newAppAndProjLister(defaultProj)

s := NewServer(&repoServerClientset, db, enforcer, newFixtures().Cache, appLister, projLister, testNamespace, settingsMgr)
resp, err := s.DeleteRepository(context.TODO(), &repository.RepoQuery{Repo: repo, AppProject: "default"})
require.NoError(t, err)
assert.Equal(t, repositorypkg.RepoResponse{}, *resp)
})
}
}
20 changes: 17 additions & 3 deletions util/git/git.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ func IsTruncatedCommitSHA(sha string) bool {

// SameURL returns whether or not the two repository URLs are equivalent in location
func SameURL(leftRepo, rightRepo string) bool {
normalLeft := NormalizeGitURL(leftRepo)
normalRight := NormalizeGitURL(rightRepo)
normalLeft := NormalizeGitURLAllowInvalid(leftRepo)
normalRight := NormalizeGitURLAllowInvalid(rightRepo)
return normalLeft != "" && normalRight != "" && normalLeft == normalRight
}

Expand All @@ -45,6 +45,16 @@ func SameURL(leftRepo, rightRepo string) bool {
// Prefer using SameURL() over this function when possible. This algorithm may change over time
// and should not be considered stable from release to release
func NormalizeGitURL(repo string) string {
return normalizeGitURL(repo, false)
}

// Similar to NormalizeGitURL, except returning a not fully normalized string when the url is invalid.
// Needed to allow a deletion of repos with invalid urls. See https://github.com/argoproj/argo-cd/issues/20921.
func NormalizeGitURLAllowInvalid(repo string) string {
return normalizeGitURL(repo, true)
Copy link
Member

Choose a reason for hiding this comment

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

I think it will be better not to add allowInvalid parameter to private function. Can we just return original "repo" in case if error happens ? In such case you will not need to modify with additional conditions normalizeGitURL function

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've made the change.

}

func normalizeGitURL(repo string, allowInvalid bool) string {
repo = strings.ToLower(strings.TrimSpace(repo))
if yes, _ := IsSSHURL(repo); yes {
if !strings.HasPrefix(repo, "ssh://") {
Expand All @@ -57,7 +67,11 @@ func NormalizeGitURL(repo string) string {
repo = strings.TrimSuffix(repo, ".git")
repoURL, err := url.Parse(repo)
if err != nil {
return ""
if allowInvalid {
return repo
} else {
return ""
}
}
normalized := repoURL.String()
return strings.TrimPrefix(normalized, "ssh://")
Expand Down