Add usage of list trees to include cluster files in deletion pr #1210

ranatrk · 2022-08-03T15:28:29Z

Fixes #747
Add usage of list trees from go-git-provider to list all files of the cluster when creating a delete pull request for cluster deletion

Depends on fluxcd/go-git-providers#153

foot

This is really clean 💯 , looking great!

The only thing is making sure we support gitlab too. It would be nice to keep go-git-providers as the interface abstraction layer over both github/gitlab (instead of e.g. branching here based on provider type, and doing ListTree for github and GetFiles(recursive=true) for gitlab.

Would it be tricky to implement ListTrees in ggp for gitlab?

cmd/clusters-service/pkg/git/git.go

cmd/clusters-service/pkg/server/clusters.go

foot · 2022-08-06T08:21:24Z

cmd/clusters-service/pkg/server/clusters_test.go

+						currFile := fakeGitProvider.originalFiles[i].Path
+						nextFile := fakeGitProvider.originalFiles[j].Path
+						return currFile < nextFile
+					})


Nice bit of logic here ✨ !

Buttt, maybe we use the similar pattern in TestCreatePullRequest and provide the expected list of commitedFiles in each test case above.

Just so we can all test it doesn't try and delete all the files in tree.

@foot isn't this the same as using the originalFiles passed to the provider? The comparison is done between that (expected) and the actual committed files

foot · 2022-08-06T08:23:52Z

cmd/clusters-service/pkg/server/clusters_test.go

+	filteredTreeEntries := make(map[string]bool)
+	for _, f := range p.originalFiles {
+		if _, ok := filteredTreeEntries[f.Path]; !ok {
+			filteredTreeEntries[f.Path] = true


Is this in case we provide duplicates in p.originalFiles? I'm not sure if we need it?

it is because the add files to commit already adds the files in the management cluster belonging to the specified cluster, which are also in the original files , so if they are found they should be ignored (for the test's case)

ranatrk · 2022-08-10T14:17:49Z

@foot updated with adding path parameter as well as removing filtering unless no cluster path is found, and simply retrieving all the cluster paths from the tree listing instead of adding the manifest file and then checking not to add it again

foot

💯 Looking great

foot · 2022-08-12T06:03:17Z

cmd/clusters-service/pkg/server/clusters.go

-			filesList = append(filesList, gitprovider.CommitFile{
-				Path:    &path,
-				Content: nil,
-			})


Also still want to delete the getClusterManifestPath here 👍

it is replaced with getClusterDirPath

Right!

There are actually 2 "places" we want to delete files from when cleaning up.

getClusterManifestPath which is usually ./clusters/management/clusters/my-cluster.yaml, It holds the cluster definition (how many nodes it has, what version of k8s it runs), this file is loaded into the management cluster. The existing deletion behaviour only tried to delete this file.

✨ New ✨ getClusterDirPath, which is something like ./clusters/default/my-cluster/**, all the files that should run on the new cluster, could be kustomizations, helmrelease etc. We write is into a different location so that it doesn't get loaded into the management cluster. It should only run on my-cluster.

E.g. here is PR creating a cluster https://github.com/wkp-example-org/capd-demo-reloaded/pull/203/files

So we still wanna delete both 👍

cmd/clusters-service/pkg/server/clusters.go

cmd/clusters-service/pkg/server/clusters_test.go

Update NewFakeGitProvider calls with missing arguments

…Request

Update adding files paths to be deleted to all be from the tree listing instead of manually adding the manifest file seperately Signed-off-by: Rana Tarek Hassan <[email protected]>

fix cluster file path in test Co-authored-by: Simon <[email protected]>

Cleanup delete cluster pull request test in clusters_test Remove filtering retrieved tree files in delete cluster pull request Signed-off-by: Rana Tarek Hassan <[email protected]>

Update delete cluster pr test with updated manifest and cluster paths Signed-off-by: Rana Tarek Hassan <[email protected]>

Signed-off-by: Rana Tarek Hassan <[email protected]>

foot

Amazing

Works great! ⭐

foot · 2022-08-23T07:41:09Z

cmd/clusters-service/pkg/server/clusters_test.go

+			name: "create delete pull request with namespaced cluster names including multiple files in tree",
+			provider: NewFakeGitProvider("https://github.com/org/repo/pull/1", nil, nil, []string{
+				"clusters/ns-foo/foo/kustomization.yaml",
+				"clusters/management/clusters/ns-foo/foo.yaml",


I guess everything coming back from getTrees should be deleted actually? I was thinking we should add some other files here which shouldn't be deleted.. but as ggp is handling all the filtering this is basically just the set of files that should be removed.

yes these are the files that are added by fake gpp so adding other files in the list of the test will actually add them in the commit to be deleted

foot · 2022-08-23T07:41:29Z

go.mod

@@ -288,6 +288,7 @@ replace (
 	// to fix a vulnerability affecting the github.com/gorilla/handlers dependency. For more info visit
 	// https://security.snyk.io/vuln/SNYK-GOLANG-GITHUBCOMGORILLAHANDLERS-540773. Newer versions _should_ also work.
 	github.com/docker/distribution => github.com/docker/distribution v0.0.0-20201106182221-03aaf6ab5111
+	github.com/fluxcd/go-git-providers => github.com/ranatrk/go-git-providers v0.0.0-20220809210816-77b521f7fbb7


Lets update this to a commit on main when the ggp PR is merged!

ranatrk added the bug Something isn't working label Aug 4, 2022

ranatrk force-pushed the remove-cluster-directories branch 3 times, most recently from c69772e to 10185c8 Compare August 4, 2022 12:35

foot reviewed Aug 6, 2022

View reviewed changes

ranatrk requested a review from foot August 10, 2022 14:26

ranatrk force-pushed the remove-cluster-directories branch from 0f6c52c to 5f16e2b Compare August 11, 2022 10:19

foot reviewed Aug 12, 2022

View reviewed changes

ranatrk and others added 9 commits August 15, 2022 13:09

Add usage of list trees to include cluster files in deletion pr

47cd792

TEMP replace go-git-provider mod to temp repo

f50d3fc

Add err check in Gettreelist

d15f2aa

Update NewFakeGitProvider calls with missing arguments

Fix TestDeleteClustersPullRequest test in clusters_test

9ff9f81

Update error messages when handeling tree files in DeleteClustersPull…

ca615f3

…Request

TEMP upgrade temp go-git-provider in temp repo

708c7c0

Add path to list git tree to filter tree entries in specific path dir

ec20291

Update adding files paths to be deleted to all be from the tree listing instead of manually adding the manifest file seperately Signed-off-by: Rana Tarek Hassan <[email protected]>

Update cmd/clusters-service/pkg/server/clusters_test.go

97e1ad8

fix cluster file path in test Co-authored-by: Simon <[email protected]>

Apply suggestions from review

d1e6721

Cleanup delete cluster pull request test in clusters_test Remove filtering retrieved tree files in delete cluster pull request Signed-off-by: Rana Tarek Hassan <[email protected]>

ranatrk force-pushed the remove-cluster-directories branch from c81619e to d1e6721 Compare August 15, 2022 11:19

Add manifest files path in delete cluster PR

7450343

Update delete cluster pr test with updated manifest and cluster paths Signed-off-by: Rana Tarek Hassan <[email protected]>

ranatrk requested a review from foot August 15, 2022 14:47

ranatrk and others added 4 commits August 15, 2022 16:55

Merge branch 'main' into remove-cluster-directories

7e0df7f

Merge branch 'main' into remove-cluster-directories

1157be3

Update clusters_test after merge from main

613a08f

Signed-off-by: Rana Tarek Hassan <[email protected]>

Merge branch 'main' into remove-cluster-directories

7ed020e

foot approved these changes Aug 23, 2022

View reviewed changes

ranatrk added 4 commits August 23, 2022 12:16

Merge branch 'main' into remove-cluster-directories

a6a1c37

Merge branch 'main' into remove-cluster-directories

76de346

Update go-git-provider to use latest v0.8.0

c1cd851

Merge branch 'main' into remove-cluster-directories

f951050

ranatrk added 3 commits August 25, 2022 13:00

Merge branch 'main' into remove-cluster-directories

8818115

Add missing arguments after merge with main

26b8d7e

Update go-git-provider to latest commit on main branch

98c9e37

ranatrk merged commit 7ef05e7 into main Aug 25, 2022

ranatrk deleted the remove-cluster-directories branch August 25, 2022 12:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add usage of list trees to include cluster files in deletion pr #1210

Add usage of list trees to include cluster files in deletion pr #1210

ranatrk commented Aug 3, 2022 •

edited

Loading

foot left a comment

foot Aug 6, 2022

ranatrk Aug 8, 2022

foot Aug 6, 2022

ranatrk Aug 9, 2022

ranatrk commented Aug 10, 2022

foot left a comment

foot Aug 12, 2022

ranatrk Aug 14, 2022

foot Aug 15, 2022

foot left a comment

foot Aug 23, 2022

ranatrk Aug 23, 2022

foot Aug 23, 2022

Add usage of list trees to include cluster files in deletion pr #1210

Add usage of list trees to include cluster files in deletion pr #1210

Conversation

ranatrk commented Aug 3, 2022 • edited Loading

foot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranatrk commented Aug 10, 2022

foot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

foot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ranatrk commented Aug 3, 2022 •

edited

Loading