fix: Cloning for environments shouldn't leave strange state in JX_HOME #6479

abayer · 2020-01-09T18:18:45Z

Submitter checklist

Change is code complete and matches issue description.
Change is covered by existing or new tests.

Description

Until this, jx delete app, jx add app, and jx get apps all did weird things with/under ~/.jx/environments. All of them used ForkAndPullRepo to clone under there - jx get apps directly into ~/.jx/environments, and jx add app and jx delete app into ~/.jx/environments/dev. Literally none of this made sense. =)

This reworks not just the app commands, but all commands that create PRs (i.e., gitops) against the dev/prod/staging environments to use temporary directories for the fork/clone/push, with the option, just used in tests, of specifying an existing directory to use instead, while jx get apps just clones to a temporary directory as well.

Switching all this to temporary directories also really helps users who manage/work with multiple JX clusters from a single laptop/desktop/host, since namespacing ~/.jx/environments by cluster is basically impossible so far as I can tell.

Also adds --auto-merge to jx delete app because I was here.

Special notes for the reviewer(s)

Which issue this PR fixes

fixes #6350

abayer · 2020-01-09T18:19:00Z

/assign @ccojocar
/assign @hferentschik
/assign @pmuir

abayer · 2020-01-09T19:06:20Z

I'm not shocked at the failures - I just wanted to push up what worked for actual apps usage before I had to run to an appointment. Will fix later today

hferentschik · 2020-01-09T19:31:19Z

pkg/apps/gitops.go

@@ -166,9 +176,18 @@ func (o *GitOpsOptions) DeleteApp(app string, alias string, autoMerge bool) erro

 // GetApps retrieves all the apps information for the given appNames from the repository and / or the CRD API
 func (o *GitOpsOptions) GetApps(appNames map[string]bool, expandFn func([]string) (*v1.AppList, error)) (*v1.AppList, error) {
-	dir, _, _, _, err := gits.ForkAndPullRepo(o.DevEnv.Spec.Source.URL, o.EnvironmentsDir, o.DevEnv.Spec.Source.Ref, "master", o.GitProvider, o.Gitter, "")
+	dir, err := ioutil.TempDir("", "create-pr")


on a side note, I think we should start using a base dir within JX_HOME, e.g. JX_HOME/tmp for all out temporary directory creations. This probably makes it easier to debug and it is also easier to see when things don't get cleaned up.

and why would 'get app' create a tmp directory with 'create-pr' in it?

So for each 'get apps' we clone the repo? That does not seem terribly effective? I guess the idea of using ~/.jx/environment was to have some sort of a cache.

Admittedly something which works it better than something which is "more performant" but not working ;-)

jx get app and jx get delete app create PRs, so...

And yeah, I decided to ditch the sorta cache in ~/.jx/environments because (a) it was horribly broken as written, and (b) there currently is no viable way to namespace the clone inside ~/.jx/environments by cluster, so if you've got multiple clusters, you'd still end up having potential problems.

wait, my brain fried - this shouldn't be create-pr-. duh. =)

hferentschik · 2020-01-09T19:39:05Z

pkg/apps/gitops.go

@@ -166,9 +176,18 @@ func (o *GitOpsOptions) DeleteApp(app string, alias string, autoMerge bool) erro

 // GetApps retrieves all the apps information for the given appNames from the repository and / or the CRD API
 func (o *GitOpsOptions) GetApps(appNames map[string]bool, expandFn func([]string) (*v1.AppList, error)) (*v1.AppList, error) {
-	dir, _, _, _, err := gits.ForkAndPullRepo(o.DevEnv.Spec.Source.URL, o.EnvironmentsDir, o.DevEnv.Spec.Source.Ref, "master", o.GitProvider, o.Gitter, "")


As much as I also dislike the use of ForAndPullRepo here, if for no other reason than the fact that it is a terrible name, I believe this would actually work, if there were not the #5772 bug.

Another issue with ~/.jx/environment is that in the case of app it is not keys against context. So if you have multiple clusters and you switch between them you also get an error, this time a legit merge error. This is due to the fact that this app code came first. Later code which uses JX_HOME to cache checkout is keying against the cluster. I believe there is an issue for this as well.

But why use ForkAndPullRepo here? It's the forking that drove me off - there's absolutely no reason to fork the dev env as part of running jx get apps.

Exactly, the name is terrible. It implies completely different semantics to what is actually happening, depending on the passed parameters. It is not always forking. The docs also says "ForkAndPullRepo pulls the specified gitUrl into dir using gitter, creating a remote fork if needed using the git provider". But that makes the function not less confusing. It is one of these cases where a single methods tries to do everything.

I agree it completely confuses the reader of the code at this point in the code. I also think there are several things wrong with ForkAndPullRepo, so the less we use it the better.

Doing a fresh throw-away checkout for each of these app operations will fix it, but with a pretty high cost. It feels like it there should be a better way. But let's go for something which works.

We already effectively are doing throw away checkouts for jx add app, jx delete app, jx upgrade app, and their friends (namely jx step scheduler [apply|migrate]. I just threw in a throwaway checkout for jx get apps too. =)

pkg/apps/gitops.go

houndci-bot · 2020-01-09T23:49:20Z

pkg/apps/gitops.go

+	if err != nil {
+		return nil, errors.Wrapf(err, "determining git provider information for %s", o.DevEnv.Spec.Source.URL)
+	}
+	cloneUrl := providerInfo.CloneURL


var cloneUrl should be cloneURL

pkg/apps/gitops.go

abayer · 2020-01-09T23:50:50Z

Fixed most of the test failures - controller_workflow_integration_test is failing but I literally have no idea how this change makes it fail and I've had a looooong day so I'll look into it tomorrow.

hferentschik · 2020-01-10T09:52:23Z

pkg/cmd/add/add_app_test.go

@@ -80,6 +80,11 @@ func TestAddAppForGitOps(t *testing.T) {
 			DevEnv:     testOptions.DevEnv,
 			HelmUpdate: true, // Flag default when run on CLI
 		}
+		envDir, err := o.CommonOptions.EnvironmentsDir()


is this test retrying!?

So if you removed the use of the EnvironmentsDir dir in the app lifecycle methods (get|delete|add) how come you need to still set it here. That seems counter-intuitive and tbh confusing.

Because the tests want to look at the contents of the resulting PR to make sure it did the right thing, and because of how FakeProvider works, that means we've gotta keep around the repo we create the PR in. Hence the CloneDir option I've added to all the various commands but not exposed via the CLI - a way to say "actually, don't use a temporary directory, use this one".

So we need to add this just for testing purposes? Seems wrong, especially since it confuses the reader of the production code. I already stumbled across your comment.

Is there no other way to test this?

Not so far as I can tell - opening a PR with FakeProvider just adds a record on the target repository's struct saying "there's a PR from this other repo", so once the other repo's directory gets deleted, the actual content of the PR is gone. So we either need to do the CloneDir stuff or we need to remove checking the content of the PR from the tests, which seems like a bad idea.

Another approach would be to work against known locations. As I mentioned before, I think we should create all these temporary directories in a common base, aka JX_HOME/tmp. This way we have a place to look/scan for the created directory.

This combined with creating the tmp directory quite high up in the call hierachy (somewhere close to Run), would probably get us closter to where we want to be.

For now that is probably too much to ask for ;-)

So we either need to do the CloneDir stuff or we need to remove checking the content of the PR from the tests, which seems like a bad idea.

Maybe the tests could be split as well. Personally, I would be very reluctant to have this flag just because I need it for testing. For me it is a code smell.

hferentschik · 2020-01-10T09:55:14Z

pkg/cmd/add/add_app_test.go

@@ -80,6 +80,11 @@ func TestAddAppForGitOps(t *testing.T) {
 			DevEnv:     testOptions.DevEnv,
 			HelmUpdate: true, // Flag default when run on CLI
 		}
+		envDir, err := o.CommonOptions.EnvironmentsDir()


So if you removed the use of the EnvironmentsDir dir in the app lifecycle methods (get|delete|add) how come you need to still set it here. That seems counter-intuitive and tbh confusing.

hferentschik · 2020-01-10T10:01:44Z

pkg/apps/gitops.go

@@ -42,6 +42,16 @@ func (o *GitOpsOptions) AddApp(app string, dir string, version string, repositor
 		GitProvider: o.GitProvider,
 	}

+	prDir := o.EnvironmentsDir


I find it very confusing that parts of the required input for this function come via function parameters and others by accessing the GitOpsOptions instance. For example, autoMerge is passed as a parameter, o.EnvironmentsDir not.

I've tidied it up a bit, but it's still kinda janky.

hferentschik · 2020-01-10T10:06:13Z

pkg/apps/gitops.go

@@ -42,6 +42,16 @@ func (o *GitOpsOptions) AddApp(app string, dir string, version string, repositor
 		GitProvider: o.GitProvider,
 	}

+	prDir := o.EnvironmentsDir
+	if prDir == "" {


This confuses me. What's happening here. So first you set the prDir to the EnvironmentsDir which I believe is ~/.jx/environments, right? If it does not exist you create a tmp directory which eventually gets deleted. I get that part, but what if ~/.jx/environments already exists?

hmm, on second thought, I might be looking at something here which is work in progress, since prDir is not even used.

Fixed that.

abayer · 2020-01-10T20:11:53Z

/retest

abayer · 2020-01-10T20:37:55Z

/retest

abayer · 2020-01-10T21:28:08Z

/retest

abayer · 2020-01-10T22:13:59Z

/test boot-vault

abayer · 2020-01-10T23:30:16Z

/retest

abayer · 2020-01-11T00:03:58Z

/retest

abayer · 2020-01-11T00:19:09Z

This will pass again once jenkins-x/bdd-jx#104 merges.

abayer · 2020-01-11T11:43:01Z

/retest

hferentschik · 2020-01-13T14:23:11Z

pkg/apps/gitops.go

+	}
+	cloneUrl := providerInfo.CloneURL
+	userDetails := o.GitProvider.UserAuth()
+	originFetchURL, err := o.Gitter.CreateAuthenticatedURL(cloneUrl, &userDetails)


:-( - this is bad. We should not use this approach. Sad to see that this proliferates now outside ForkAndPullRepo

I don't disagree, but I couldn't figure out how to ensure that ForkAndPullRepo doesn't fork (it seems to fork if the current user is different from the owner of the repo) and I needed an authenticated clone URL, so...

Though I'd be fine with adding an argument to ForkAndPullRepo to explicitly block forking and just using it here.

hferentschik · 2020-01-13T14:24:58Z

pkg/apps/install.go

@@ -175,6 +175,12 @@ func (o *InstallOptions) DeleteApp(app string, alias string, releaseName string,
 		Items: make([]string, 0),
 	}

+	// Make sure that we use a temporary directory for GetApps even if we're not using a temporary directory for DeleteApps


What does that mean? Thought we are going to use tmp directories for all of it now?

That's an out-of-date comment - I'll remove.

...and out of date code. Fixing. =)

hferentschik · 2020-01-13T14:27:23Z

pkg/cmd/add/add_app_test.go

@@ -80,6 +80,11 @@ func TestAddAppForGitOps(t *testing.T) {
 			DevEnv:     testOptions.DevEnv,
 			HelmUpdate: true, // Flag default when run on CLI
 		}
+		envDir, err := o.CommonOptions.EnvironmentsDir()


So we need to add this just for testing purposes? Seems wrong, especially since it confuses the reader of the production code. I already stumbled across your comment.

Is there no other way to test this?

hferentschik · 2020-01-13T14:27:56Z

pkg/cmd/deletecmd/delete_app.go

+	AutoMerge   bool
+
+	// Used for testing
+	CloneDir string


Until this, `jx delete app`, `jx add app`, and `jx get apps` all did weird things with/under `~/.jx/environments`. All of them used `ForkAndPullRepo` to clone under there - `jx get apps` directly into `~/.jx/environments`, and `jx add app` and `jx delete app` into `~/.jx/environments/dev`. Literally none of this made sense. =) This reworks not just the app commands, but all commands that create PRs (i.e., gitops) against the dev/prod/staging environments to use temporary directories for the fork/clone/push, with the option, just used in tests, of specifying an existing directory to use instead, while `jx get apps` just clones to a temporary directory as well. Switching all this to temporary directories also really helps users who manage/work with multiple JX clusters from a single laptop/desktop/host, since namespacing `~/.jx/environments` by cluster is basically impossible so far as I can tell. Also adds `--auto-merge` to `jx delete app` because I was here. fixes jenkins-x#6350 Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

always be temporary. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

hferentschik · 2020-01-13T19:09:54Z

pkg/apps/gitops.go

@@ -166,9 +167,37 @@ func (o *GitOpsOptions) DeleteApp(app string, alias string, autoMerge bool) erro

 // GetApps retrieves all the apps information for the given appNames from the repository and / or the CRD API
 func (o *GitOpsOptions) GetApps(appNames map[string]bool, expandFn func([]string) (*v1.AppList, error)) (*v1.AppList, error) {
-	dir, _, _, _, err := gits.ForkAndPullRepo(o.DevEnv.Spec.Source.URL, o.EnvironmentsDir, o.DevEnv.Spec.Source.Ref, "master", o.GitProvider, o.Gitter, "")
+	// AddApp, DeleteApp, and UpgradeApps delegate selecting/creating the directory to clone in to environments/gitops.go's
+	// Create function, but here we need to create the directory explicitly. since we aren't calling Create, because we're


hferentschik · 2020-01-13T19:13:41Z

pkg/cmd/add/add_app_test.go

@@ -80,6 +80,11 @@ func TestAddAppForGitOps(t *testing.T) {
 			DevEnv:     testOptions.DevEnv,
 			HelmUpdate: true, // Flag default when run on CLI
 		}
+		envDir, err := o.CommonOptions.EnvironmentsDir()


Another approach would be to work against known locations. As I mentioned before, I think we should create all these temporary directories in a common base, aka JX_HOME/tmp. This way we have a place to look/scan for the created directory.

This combined with creating the tmp directory quite high up in the call hierachy (somewhere close to Run), would probably get us closter to where we want to be.

For now that is probably too much to ask for ;-)

hferentschik · 2020-01-13T19:15:45Z

pkg/cmd/add/add_app_test.go

@@ -80,6 +80,11 @@ func TestAddAppForGitOps(t *testing.T) {
 			DevEnv:     testOptions.DevEnv,
 			HelmUpdate: true, // Flag default when run on CLI
 		}
+		envDir, err := o.CommonOptions.EnvironmentsDir()


So we either need to do the CloneDir stuff or we need to remove checking the content of the PR from the tests, which seems like a bad idea.

Maybe the tests could be split as well. Personally, I would be very reluctant to have this flag just because I need it for testing. For me it is a code smell.

hferentschik · 2020-01-13T19:26:01Z

/lgtm

jenkins-x-bot · 2020-01-13T19:26:16Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hferentschik

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [hferentschik]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

jenkins-x-bot requested review from hferentschik and romainverduci January 9, 2020 18:18

jenkins-x-bot assigned ccojocar, hferentschik and pmuir Jan 9, 2020

jenkins-x-bot added the size/M label Jan 9, 2020

hferentschik reviewed Jan 9, 2020

View reviewed changes

abayer force-pushed the apps-and-gitops-stop-that-env-dir branch from a2dc804 to aed7821 Compare January 9, 2020 23:49

houndci-bot reviewed Jan 9, 2020

View reviewed changes

pkg/apps/gitops.go Show resolved Hide resolved

houndci-bot reviewed Jan 9, 2020

View reviewed changes

pkg/apps/gitops.go Show resolved Hide resolved

houndci-bot reviewed Jan 9, 2020

View reviewed changes

pkg/apps/gitops.go Show resolved Hide resolved

jenkins-x-bot added size/L and removed size/M labels Jan 9, 2020

hferentschik reviewed Jan 10, 2020

View reviewed changes

abayer changed the title ~~fix: Environment cloning for app commands shouldn't leave strange state in JX_HOME~~ WIP: fix: Environment cloning for app commands shouldn't leave strange state in JX_HOME Jan 10, 2020

jenkins-x-bot added the do-not-merge/work-in-progress label Jan 10, 2020

abayer force-pushed the apps-and-gitops-stop-that-env-dir branch 2 times, most recently from 540bba0 to 833a165 Compare January 10, 2020 17:11

abayer changed the title ~~WIP: fix: Environment cloning for app commands shouldn't leave strange state in JX_HOME~~ fix: Cloning for environments shouldn't leave strange state in JX_HOME Jan 10, 2020

jenkins-x-bot removed the do-not-merge/work-in-progress label Jan 10, 2020

abayer force-pushed the apps-and-gitops-stop-that-env-dir branch from 4c08096 to 2d445e5 Compare January 10, 2020 23:27

abayer force-pushed the apps-and-gitops-stop-that-env-dir branch from 2d445e5 to e329377 Compare January 11, 2020 00:18

hferentschik reviewed Jan 13, 2020

View reviewed changes

hferentschik suggested changes Jan 13, 2020

View reviewed changes

abayer force-pushed the apps-and-gitops-stop-that-env-dir branch from e329377 to 66d04f7 Compare January 13, 2020 14:42

fix: GetApps doesn't need to preserve its clone directory so can

d661a68

always be temporary. Signed-off-by: Andrew Bayer <andrew.bayer@gmail.com>

hferentschik approved these changes Jan 13, 2020

View reviewed changes

jenkins-x-bot added lgtm approved labels Jan 13, 2020

jenkins-x-bot merged commit a915485 into jenkins-x:master Jan 13, 2020

fix: Cloning for environments shouldn't leave strange state in JX_HOME #6479

fix: Cloning for environments shouldn't leave strange state in JX_HOME #6479

Conversation

abayer commented Jan 9, 2020 • edited Loading

Submitter checklist

Description

Special notes for the reviewer(s)

Which issue this PR fixes

abayer commented Jan 9, 2020

abayer commented Jan 9, 2020

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

abayer commented Jan 9, 2020

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

abayer commented Jan 10, 2020

abayer commented Jan 10, 2020

abayer commented Jan 10, 2020

abayer commented Jan 10, 2020

abayer commented Jan 10, 2020

abayer commented Jan 11, 2020

abayer commented Jan 11, 2020

abayer commented Jan 11, 2020

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

hferentschik commented Jan 13, 2020

jenkins-x-bot commented Jan 13, 2020

abayer commented Jan 9, 2020 •

edited

Loading