Cluster overrides functionality in gen2 clusters #364

jayanth-tjvrr · 2022-12-12T19:30:13Z

Fixes #111
This PR adds cluster overrides functionality to gen2 clusters. A user can now generate a cluster with changes from a single base manifest without the need to change the original manifest. The design doc of the overrides: overrides-doc
The cluster which are deployed using patches( with --override flag) are turning out to be healthy as well.

…design

bcle · 2022-12-14T00:02:13Z

cmd/cluster/cluster.go

@@ -17,7 +10,7 @@ func NewCommand() *cobra.Command {
 		Short:             "Manage clusters",
 		Long:              "Manage clusters",
 		DisableAutoGenTag: true,
-		PersistentPreRun:  checkForArgocd,
+		//PersistentPreRun:  checkForArgocd,


bcle · 2022-12-14T00:02:22Z

cmd/cluster/cluster.go

-		os.Exit(1)
-	}
-}
+// func checkForArgocd(c *cobra.Command, args []string) {


delete if unused

bcle · 2022-12-14T00:05:51Z

cmd/cluster/create.go

@@ -55,6 +63,19 @@ func createClusterCommand() *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("failed to get repository credentials: %s", err)
 			}
+			overRiden := false


Spelling is incorrect. Rename to overridden

I corrected the spelling everywhere in the code now.

bcle · 2022-12-14T00:06:24Z

cmd/cluster/create.go

 	var clusterRepoPath string
 	var clusterName string
+	var overRides string


Naming is weird. Rename to overrides.

bcle · 2022-12-14T00:07:15Z

cmd/cluster/create.go

@@ -55,6 +63,19 @@ func createClusterCommand() *cobra.Command {
 			if err != nil {
 				return fmt.Errorf("failed to get repository credentials: %s", err)
 			}
+			overRiden := false
+			if overRides != " " {


This looks wrong. There's a space between the quotes.

removed the space between quotes

bcle · 2022-12-14T02:17:48Z

pkg/gitutils/gitutils.go

@@ -58,3 +59,29 @@ func CommitChanges(tmpDir string, wt *gogit.Worktree, commitMsg string) (changed
 	}
 	return
 }
+
+func CommitDeletechanges(tmpDir string, wt *gogit.Worktree, commitMsg string) (changed bool, err error) {


Rename: CommitDeleteChanges

How is this different from CommitChanges()? If the differences are none or minimal, consider consolidating into one function.

CommitChanges() function doesn't find the files which have been deleted and hence, it shows that there is nothing to commit even if few files have been deleted. Whereas the CommitDeleteChanges() finds the files which have been deleted and add and commit them.

pkg/gitutils/manifests.go

bcle · 2022-12-14T02:28:37Z

pkg/gitutils/manifests.go

+			return fmt.Errorf("failed to open embedded file %s: %s", filePath, err)
+		}
+
+		kustomfile, err := ioutil.ReadFile(newFilePath)


Suggested naming: kustomFile

Yes, I renamed all the other variables as well

bcle · 2022-12-14T02:29:01Z

pkg/gitutils/manifests.go

+		var targetcomp []string
+		var kind string
+		var information info
+		for kustomkey, kustomval := range parsedData {


Suggested naming: kustomKey, kustomVal. Same thing for others like targetcomp and strkustomval.

…ch repo path)

cmd/cluster/create.go

bcle · 2022-12-15T20:21:42Z

pkg/cluster/cluster_app.go

 ) (*argoappv1.Application, error) {
 	app := constructClusterApp(argocdNs, clusterName, baseClusterName,
-		repoUrl, repoRevision, repoPath)
+		repoUrl, repoRevision, repoPath, overRiden)


Rename to overridden

bcle · 2022-12-15T20:24:03Z

pkg/cluster/cluster_app.go

 ) *argoappv1.Application {
+	clusterOverridden := "false"


This is cleaner: clusterOverridden := fmt.Sprintf("%v", overridden)

pkg/cluster/delete.go

pkg/cluster/git.go

bcle

The functionality that copies / transforms the patch files from the patch directory to the destination git directory is a good candidate for a unit test. See if you can add a unit test for it.

bcle · 2022-12-16T01:42:53Z

pkg/cluster/delete.go

@@ -126,3 +90,57 @@ func Delete(
 	}
 	return nil
 }
+
+func DeleteOverrideDir(appIf argoapp.ApplicationServiceClient, kubeClient *kubernetes.Clientset, argocdNs string, name string) error {


A few nits:

Rename the function to DeleteOverridesDir()

Rename name to clusterName since it's not clear what thing we're talking about.

Renamed both the fields

bcle · 2022-12-16T01:43:21Z

pkg/cluster/delete.go

 			if err != nil {
-				return fmt.Errorf("failed to push to remote repository: %s", err)
+				return fmt.Errorf("failed to delete the override directory: %s", err)


"overrides directory"

bcle · 2022-12-16T01:48:43Z

pkg/cluster/delete.go

 	clusterQuery := "arlon-cluster=" + name
 	apps, err := appIf.List(context.Background(),
 		&argoapp.ApplicationQuery{Selector: &clusterQuery})
 	if err != nil {
 		return fmt.Errorf("failed to list apps related to cluster: %s", err)
 	}
+
+	app, err := appIf.Get(context.Background(),


It's unnecessary to get the "cluster-app" app here, because line 53 already gets all apps related to this cluster, and lines 77-92 iterate over those apps. So you should insert code in that loop to catch the one of type cluster-app, and then handle it.

removed the unnecessary calling of appIf.List again and used the loop in lines 77-92 to do the same

bcle · 2022-12-16T01:49:31Z

pkg/cluster/delete.go

+
+func DeleteOverrideDir(appIf argoapp.ApplicationServiceClient, kubeClient *kubernetes.Clientset, argocdNs string, name string) error {
+	log := logpkg.GetLogger()
+	app, err := appIf.Get(context.Background(),


This function should not need to get the app again. The caller already has a copy of the app. Pass it by pointer.

Passed the app pointer instead of appIf

bcle · 2022-12-16T01:50:14Z

pkg/cluster/delete.go

+		return fmt.Errorf("failed to get argocd application: %s", err)
+	}
+	typ := app.Labels["arlon-type"]
+	if typ == "cluster-app" {


There's no need to check the type if we assume the caller has already done so. You can:

Not check

Still check, and return an error if it's not of the expected type.

pkg/cluster/delete.go

bcle · 2022-12-16T01:51:15Z

pkg/cluster/types.go

@@ -15,6 +15,7 @@ type BaseClusterInfo struct {
 	RepoUrl      string
 	RepoRevision string
 	RepoPath     string
+	OverRidden   string


Rename to overridden

bcle · 2022-12-16T01:53:16Z

pkg/gitutils/manifests.go

+		src, err := os.OpenFile(newFilePath, os.O_RDONLY, os.ModePerm)
+		if err != nil {
+			_ = src.Close()
+			return fmt.Errorf("failed to open embedded file %s: %s", filePath, err)


Why does this say embedded? Should it say patch file instead?

renamed it to patch file

bcle · 2022-12-16T01:53:52Z

pkg/gitutils/manifests.go

+		kustomFile, err := ioutil.ReadFile(newFilePath)
+		if err != nil {
+			_ = src.Close()
+			return fmt.Errorf("Failed to read the embedded file %s", err)


Same comment about "embedded" terminology.

pkg/gitutils/manifests.go

bcle

Some missing error checks.

pkg/cluster/delete.go

bcle

One more minor code change.

bcle · 2022-12-16T06:59:47Z

pkg/cluster/delete.go

+	}
+	wt, err := repo.Worktree()
+	fileInfo, err := wt.Filesystem.Lstat(repoPath)
+	if err == nil {


It's more elegant to check for err != nil and return early (see line 112). This way, lines 104-110 can be re-indented and aligned with the rest of the function body.

Yes, I'm checking for err!=nil case first now

* Added overrides support for gen2 clusters * refactored the code to make the override feature work on existing bc design * Added a check if the cluster already exists before creating patch file * Added documentation for clusteroverides design proposal * Added override in user facing docs(gen2_Tutorial.md) * Addressed all the reviews(Cleared confusion in patch revision and patch repo path) * Edited the desc for patchreporevision * Addressed the reviews(Code refactoring) * Removed unnecessary code * renamed overriden * Handled the errors in delete function * Refactored the code Co-authored-by: Jayanth Reddy <[email protected]>

jayanth-tjvrr temporarily deployed to arlon December 12, 2022 19:30 — with GitHub Actions Inactive

jayanth-tjvrr temporarily deployed to arlon December 13, 2022 09:17 — with GitHub Actions Inactive

Jayanth Reddy added 4 commits December 13, 2022 14:50

Added overrides support for gen2 clusters

6da5cc3

refactored the code to make the override feature work on existing bc …

f5f8501

…design

Added a check if the cluster already exists before creating patch file

959ab05

Added documentation for clusteroverides design proposal

ffdc923

jayanth-tjvrr force-pushed the private/main/jayanth-reddy/clusteroverride-gen2clusters branch from da1f438 to ffdc923 Compare December 13, 2022 09:21

jayanth-tjvrr temporarily deployed to arlon December 13, 2022 09:21 — with GitHub Actions Inactive

jayanth-tjvrr marked this pull request as ready for review December 13, 2022 09:22

jayanth-tjvrr requested review from ShaunakJoshi1407, Rohitrajak1807, bcle and cruizen December 13, 2022 09:22

Rohitrajak1807 assigned jayanth-tjvrr Dec 13, 2022

Added override in user facing docs(gen2_Tutorial.md)

0ebfb60

jayanth-tjvrr temporarily deployed to arlon December 13, 2022 12:37 — with GitHub Actions Inactive

bcle requested changes Dec 14, 2022

View reviewed changes

Addressed all the reviews(Cleared confusion in patch revision and pat…

f166b13

…ch repo path)

jayanth-tjvrr temporarily deployed to arlon December 14, 2022 19:16 — with GitHub Actions Inactive

jayanth-tjvrr requested a review from bcle December 14, 2022 19:32

Edited the desc for patchreporevision

18512a1

jayanth-tjvrr temporarily deployed to arlon December 14, 2022 19:47 — with GitHub Actions Inactive

bcle requested changes Dec 15, 2022

View reviewed changes

Addressed the reviews(Code refactoring)

3dda237

jayanth-tjvrr temporarily deployed to arlon December 16, 2022 01:28 — with GitHub Actions Inactive

jayanth-tjvrr requested a review from bcle December 16, 2022 01:30

bcle requested changes Dec 16, 2022

View reviewed changes

Removed unnecessary code

c751ea2

jayanth-tjvrr temporarily deployed to arlon December 16, 2022 02:43 — with GitHub Actions Inactive

jayanth-tjvrr requested a review from bcle December 16, 2022 02:43

renamed overriden

a0ff317

jayanth-tjvrr temporarily deployed to arlon December 16, 2022 02:54 — with GitHub Actions Inactive

bcle requested changes Dec 16, 2022

View reviewed changes

pkg/cluster/delete.go Show resolved Hide resolved

pkg/cluster/delete.go Show resolved Hide resolved

pkg/cluster/delete.go Show resolved Hide resolved

Handled the errors in delete function

d36fd08

jayanth-tjvrr temporarily deployed to arlon December 16, 2022 03:20 — with GitHub Actions Inactive

jayanth-tjvrr requested a review from bcle December 16, 2022 03:21

bcle requested changes Dec 16, 2022

View reviewed changes

Refactored the code

2e5f626

jayanth-tjvrr temporarily deployed to arlon December 16, 2022 07:07 — with GitHub Actions Inactive

jayanth-tjvrr requested a review from bcle December 16, 2022 07:09

bcle approved these changes Dec 16, 2022

View reviewed changes

jayanth-tjvrr merged commit 0fc1274 into main Dec 16, 2022

Rohitrajak1807 deleted the private/main/jayanth-reddy/clusteroverride-gen2clusters branch December 22, 2022 17:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cluster overrides functionality in gen2 clusters #364

Cluster overrides functionality in gen2 clusters #364

jayanth-tjvrr commented Dec 12, 2022 •

edited

Loading

bcle Dec 14, 2022

bcle Dec 14, 2022

bcle Dec 14, 2022

jayanth-tjvrr Dec 14, 2022

bcle Dec 14, 2022

bcle Dec 14, 2022

jayanth-tjvrr Dec 14, 2022

bcle Dec 14, 2022

jayanth-tjvrr Dec 14, 2022

bcle Dec 14, 2022

jayanth-tjvrr Dec 14, 2022

bcle Dec 14, 2022

bcle Dec 15, 2022

bcle Dec 15, 2022

bcle left a comment

bcle Dec 16, 2022

jayanth-tjvrr Dec 16, 2022

bcle Dec 16, 2022

bcle Dec 16, 2022

jayanth-tjvrr Dec 16, 2022

bcle Dec 16, 2022

jayanth-tjvrr Dec 16, 2022

bcle Dec 16, 2022

bcle Dec 16, 2022

bcle Dec 16, 2022

jayanth-tjvrr Dec 16, 2022

bcle Dec 16, 2022

bcle left a comment

bcle left a comment

bcle Dec 16, 2022

jayanth-tjvrr Dec 16, 2022

Cluster overrides functionality in gen2 clusters #364

Cluster overrides functionality in gen2 clusters #364

Conversation

jayanth-tjvrr commented Dec 12, 2022 • edited Loading

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

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

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

bcle 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bcle left a comment

Choose a reason for hiding this comment

bcle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jayanth-tjvrr commented Dec 12, 2022 •

edited

Loading