Fixes odo update from git to binary/local and vice versa #518

mik-dass · 2018-06-07T15:54:00Z

To achieve this, two functions have been added SetupForSupervisor and CleanupAfterSupervisor in occlient.go. Also some other functions are added in occlient to add and remove initContainers, volumeMounts and volumes to the deploymentConfig. Also updateDcAnnotations in occlient is added to update the annotations in the deploymentConfig.

Issue: #439
Signed-off-by: mdas mrinald7@gmail.com

kadel · 2018-06-11T07:17:24Z

pkg/component/component.go

 	var err error
 	projectName := client.GetCurrentProjectName()
+	from, _, ctype, err := GetComponentSource(client, componentName, applicationName, projectName)


there is a function to return Component type - GetComponentType. You shouldn't modify GetComponentSource just to get this information.

@kadel Ok sorry forgot that there was a function for getting the component type

kadel

URLs are deleted after the component is updated

mik-dass · 2018-06-12T10:52:00Z

@kadel Ok What are the other components that need to be preserved before the deletion?

kadel · 2018-06-12T10:53:02Z

@kadel Ok What are the other components that need to be preserved before the deletion?

everything ;-)

kadel · 2018-06-12T10:54:29Z

Also after running an update from 'local' to 'binary' DeploymentConfig still has annotations as 'local' component.

mik-dass · 2018-06-12T10:59:29Z

@kadel I mean storage, urls and? I might be forgetting more

kadel · 2018-06-12T11:40:04Z

@kadel I mean storage, urls and? I might be forgetting more

I think that is all. As rule you shouldn't touch anything expect DeploymentConfigs and BuildConfigs

mik-dass · 2018-06-20T14:31:10Z

@kadel Done. I have updated the approach and now it does not delete the component before the update instead it updates the buildConfig, deploymentConfigs and triggers the build. Please have a look.

kadel

It looks like it works, but to make sure we need to have a good test suite probably (e2e) that will test all possible update combinations.

This also needs unit tests.

kadel · 2018-06-21T14:23:14Z

pkg/occlient/occlient.go

+// AddBootstrapInitContainer adds the bootstrap init container to the deployment config
+// dc is the deployment config to be updated
+// dcName is the name of the deployment config
+func (c *Client) AddBootstrapInitContainer(dc *appsv1.DeploymentConfig, dcName string) {


Does this function need Client receiver?

No, I guess, I will remove it. Also should I make it private?

kadel · 2018-06-21T14:24:31Z

pkg/occlient/occlient.go

+// dc is the deployment config to be updated
+// dcName is the name of the deployment config
+func (c *Client) AddBootstrapInitContainer(dc *appsv1.DeploymentConfig, dcName string) {
+	appRootVolumeName := fmt.Sprintf("%s-s2idata", dcName)


appRootVolumeName is constructed in the multiple places. Could we generate this name in one place?

OK I will create a private function to fetch this

kadel · 2018-06-21T14:25:49Z

pkg/occlient/occlient.go

+// AddBootstrapVolume adds the bootstrap volume to the deployment config
+// dc is the deployment config to be updated
+// dcName is the name of the deployment config
+func (c *Client) AddBootstrapVolume(dc *appsv1.DeploymentConfig, dcName string) {


Does this function need Client receiver?

No, I guess, I will remove it. Also should I make it private?

kadel · 2018-06-21T14:25:54Z

pkg/occlient/occlient.go

+// AddBootstrapVolumeMount mounts the bootstrap volume to the deployment config
+// dc is the deployment config to be updated
+// dcName is the name of the deployment config
+func (c *Client) AddBootstrapVolumeMount(dc *appsv1.DeploymentConfig, dcName string) {


Does this function need Client receiver?

No, I guess, I will remove it. Also should I make it private?

kadel · 2018-06-21T14:27:58Z

pkg/occlient/occlient.go

 // UpdateBuildConfig updates the BuildConfig file
 // buildConfigName is the name of the BuildConfig file to be updated
 // projectName is the name of the project
 // gitUrl equals to the git URL of the source and is equals to "" if the source is of type dir or binary
 // annotations contains the annotations for the BuildConfig file
 func (c *Client) UpdateBuildConfig(buildConfigName string, projectName string, gitUrl string, annotations map[string]string) error {
+	const bootstrapperURI = "https://github.com/kadel/bootstrap-supervisored-s2i"


this is duplicate, it is already defined in Boostrap function

Should I make it a global variable?

kadel · 2018-06-21T14:47:11Z

pkg/occlient/occlient.go

@@ -876,6 +910,81 @@ func (c *Client) UpdateBuildConfig(buildConfigName string, projectName string, g
 	return nil
 }

+// AddSupervisorToDC adds the supervisor to the deployment config


I'm not sure that this is the correct name for this function. It doesn't add a supervisor to the deployment config. The supervisor is added to the image via BuildConfig, this just adds initContainer and volume that is needed for it work properly

@kadel Can you suggest any name? I am confused

kadel · 2018-06-21T14:47:47Z

pkg/occlient/occlient.go

+// builderImage is the name of the builderImage type
+// projectName is the name of the project
+// annotations are the updated annotations for the new deployment config
+func (c *Client) RemoveSupervisorFromDC(dcName string, builderImage string, projectName string, annotations map[string]string) error {


similar concern as with AddSupervisorToDC

@kadel Can you suggest any name? I am confused here too

kadel · 2018-06-26T15:28:20Z

tests/e2e/e2e_test.go

+					"{{.name}}{{end}}{{end}}'")
+				Expect(getDc).To(ContainSubstring("wildfly" + appRootVolumeName))
+
+				// checking for source-type in dc


You could create function rather than copy/pasting those 4 check this to every test ;-)

ok good idea 👍

kadel · 2018-07-13T07:46:35Z

Makefile

@@ -79,6 +79,10 @@ test-e2e:
 test-demo:
 	go test -v github.com/redhat-developer/odo/tests/e2e --ginkgo.focus="katacodaDemo" -ginkgo.v

+.PHONY: test-update-e2e
+test-update-e2e:


I suspect that those tests are never run on travis :-(

Yep, need to make a travis.yml entry for this. Something like this: https://github.com/redhat-developer/odo/blob/master/.travis.yml#L46

@kadel @ashetty1 Done, it's running separately as a separate job now

@mik-dass Fantastic!

cdrage · 2018-07-16T16:44:47Z

New changes LGTM!

kadel · 2018-07-13T07:48:56Z

pkg/component/component.go

@@ -330,7 +330,7 @@ func GetComponentSource(client *occlient.Client, componentName string, applicati
 // Component name is the name component to be updated
 // to indicates what type of source type the component source is changing to e.g from git to local or local to binary


arguments for this function were changes, comment needs to reflect that

kadel · 2018-07-13T07:51:49Z

pkg/occlient/occlient.go

@@ -55,6 +55,9 @@ const (

 	// The length of the string to be generated for names of resources
 	nameLength = 5
+	// git repository that will be used for bootstraping
+	bootstrapperURI = "https://github.com/kadel/bootstrap-supervisored-s2i"
+	bootstrapperRef = "v0.0.1"


we are using v0.0.2 now, why is this downgraded?

kadel · 2018-07-18T11:50:54Z

pkg/component/component.go

-		annotations[componentSourceTypeAnnotation] = to
-		err = client.UpdateBuildConfig(namespacedOpenShiftObject, projectName, "", annotations)
-	}
+	log.Debugf("Updating component %s, to %s (%s).", componentName, newSource, newSourceType)


Shouldn't it be:
"Updating component from %s to %s"

kadel · 2018-07-18T11:53:23Z

pkg/occlient/occlient.go

+
+// SetupForSupervisor adds the supervisor to the deployment config
+// dcName is the name of the deployment config to be updated
+// builderImage is the name of the builderImage type


there is no "builderImage" in this function

kadel · 2018-07-18T11:53:33Z

pkg/occlient/occlient.go

+
+// CleanupAfterSupervisor removes the supervisor from the deployment config
+// dcName is the name of the deployment config to be updated
+// builderImage is the name of the builderImage type


there is no "builderImage" in this function

kadel · 2018-07-18T11:55:29Z

pkg/occlient/occlient.go

+// builderImage is the name of the builderImage type
+// projectName is the name of the project
+// annotations are the updated annotations for the new deployment config
+func (c *Client) SetupForSupervisor(dcName string, projectName string, annotations map[string]string, labels map[string]string) error {


what is labels argument?
It would be explained why it is used only in CreatePVC

kadel · 2018-07-18T11:56:10Z

tests/e2e/e2e_test.go

@@ -103,6 +103,24 @@ func waitForCmdOut(cmd string, expOut string) bool {
 	}
 }

+func SourceTest(appTestName string, source string, sourceType string) {


needs comment explaining what this function is for

kadel · 2018-07-18T12:31:03Z

tests/e2e/e2e_test.go

@@ -422,3 +449,314 @@ var _ = Describe("odoe2e", func() {
 		})
 	})
 })
+
+var _ = Describe("updateE2e", func() {
+	appTestName := "testing"


shouldn't be this and following variables constants?

mik-dass · 2018-07-18T16:11:38Z

@kadel I have made the changes, please have a look again

kadel

last comments ;-)

kadel · 2018-07-19T08:15:11Z

tests/e2e/e2e_test.go

+
+var _ = Describe("updateE2e", func() {
+
+	bootStrapSupervisorURI := "https://github.com/kadel/bootstrap-supervisored-s2i"


Shouldn't this variables be constant?

kadel · 2018-07-19T08:16:25Z

tests/e2e/e2e_test.go

+		})
+
+		It("should update component from binary to local", func() {
+			runCmd("git clone https://github.com/marekjelen/katacoda-odo-backend " +


Why are you not using wildflyUri1?`

kadel · 2018-07-19T08:16:39Z

tests/e2e/e2e_test.go

+		})
+
+		It("should update component from local to git", func() {
+			runCmd("odo update wildfly --git https://github.com/marekjelen/katacoda-odo-backend")


Why are you not using wildflyUri1?

kadel · 2018-07-19T08:16:51Z

tests/e2e/e2e_test.go

+		})
+
+		It("should update component from binary to git", func() {
+			runCmd("odo update wildfly --git https://github.com/marekjelen/katacoda-odo-backend")


Why are you not using wildflyUri1?

kadel · 2018-07-19T08:17:21Z

tests/e2e/e2e_test.go

+		})
+
+		It("should update component from local to local", func() {
+			runCmd("git clone https://github.com/mik-dass/katacoda-odo-backend " +


Why are you not using wildflyUri2?

kadel · 2018-07-19T08:17:27Z

tests/e2e/e2e_test.go

+		})
+
+		It("should update component from git to git", func() {
+			runCmd("odo update wildfly --git https://github.com/mik-dass/katacoda-odo-backend")


Why are you not using wildflyUri2?

mik-dass · 2018-07-19T09:21:43Z

@kadel Done

To achieve this, two functions have been added SetupForSupervisor and CleanupAfterSupervisor in occlient.go. Also some other functions are added in occlient to add and remove initContainers, volumeMounts and volumes to the deploymentConfig. Also updateDcAnnotations in occlient is added to update the annotations in the deploymentConfig. Signed-off-by: mdas <mrinald7@gmail.com>

cdrage · 2018-07-19T16:12:34Z

@mik-dass Please update the commit description as well as the PR description before this is merged 👍

kadel

LGTM
Great work @mik-dass! 🎉

@cdrage Feel free to merge it once your concerns are addressed.

cdrage · 2018-07-20T14:59:46Z

@kadel I have edited @mik-dass 's PR description with what he put in the commit description!

To achieve this, two functions have been added SetupForSupervisor and CleanupAfterSupervisor in occlient.go. Also some other functions are added in occlient to add and remove initContainers, volumeMounts and volumes to the deploymentConfig. Also updateDcAnnotations in occlient is added to update the annotations in the deploymentConfig.

Mergin' time

kadel · 2018-07-25T16:38:10Z

pkg/occlient/occlient_test.go

+					t.Errorf("created PVC spec not matching with expected values, expected: %s, got %s", createdPVC.Spec, tt.createdPVC.Spec)
+				}
+			} else if err == nil && tt.wantErr {
+				t.Errorf("test failed, expected: %s, got %s", "error", "no error")


I'm assuming you're refereing for the part that it should be err and tt.wantErr for the "error" and "no error" parts?

kadel · 2018-07-25T16:38:24Z

pkg/occlient/occlient_test.go

+			} else if err == nil && tt.wantErr {
+				t.Errorf("test failed, expected: %s, got %s", "error", "no error")
+			} else if err != nil && !tt.wantErr {
+				t.Errorf("test failed, expected: %s, got %s", "no error", "error")


mik-dass · 2018-07-26T09:48:45Z

@kadel #605 will solve this issue

kadel reviewed Jun 11, 2018

View reviewed changes

mik-dass force-pushed the update_fix branch from e38dbe3 to 2ebc30e Compare June 11, 2018 10:45

kadel requested changes Jun 12, 2018

View reviewed changes

mik-dass added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. Required by Prow. label Jun 13, 2018

mik-dass force-pushed the update_fix branch 2 times, most recently from 72626fa to 69e1d26 Compare June 20, 2018 14:19

mik-dass changed the title ~~Trying updating the component by first deleting~~ Fixes odo update from git to binary/local and vice versa Jun 20, 2018

mik-dass mentioned this pull request Jun 21, 2018

WIP: Fix 'odo update' (updating from local/binary to git and back) #477

Closed

kadel reviewed Jun 21, 2018

View reviewed changes

mik-dass force-pushed the update_fix branch 7 times, most recently from 45fc5a8 to 880796d Compare June 26, 2018 07:37

mik-dass mentioned this pull request Jun 26, 2018

Updating th oc cluster version #540

Merged

mik-dass force-pushed the update_fix branch 3 times, most recently from d30eb22 to a26749c Compare June 26, 2018 12:10

kadel reviewed Jun 26, 2018

View reviewed changes

mik-dass force-pushed the update_fix branch 3 times, most recently from 5581d81 to 7e04722 Compare June 27, 2018 09:49

mik-dass force-pushed the update_fix branch 4 times, most recently from a644bd9 to 52d62b8 Compare July 12, 2018 16:33

kadel reviewed Jul 13, 2018

View reviewed changes

mik-dass force-pushed the update_fix branch from 52d62b8 to 762f13f Compare July 13, 2018 08:44

cdrage approved these changes Jul 16, 2018

View reviewed changes

kadel reviewed Jul 18, 2018

View reviewed changes

mik-dass force-pushed the update_fix branch 3 times, most recently from 7cb69d8 to eccf6e0 Compare July 18, 2018 16:09

kadel reviewed Jul 19, 2018

View reviewed changes

mik-dass force-pushed the update_fix branch from eccf6e0 to ba12cf6 Compare July 19, 2018 09:21

mik-dass force-pushed the update_fix branch from ba12cf6 to becca9e Compare July 19, 2018 09:33

mik-dass force-pushed the update_fix branch from becca9e to c2ccc3b Compare July 19, 2018 09:43

kadel approved these changes Jul 20, 2018

View reviewed changes

kadel reviewed Jul 20, 2018

View reviewed changes

cdrage merged commit 44e6f61 into redhat-developer:master Jul 20, 2018

ashetty1 mentioned this pull request Jul 23, 2018

BuildSource is set to git even when gitUrl is not passed #594

Closed

mik-dass deleted the update_fix branch July 25, 2018 08:37

mik-dass mentioned this pull request Jul 25, 2018

odo update fails if I use source as git first and then local #439

Closed

kadel reviewed Jul 25, 2018

View reviewed changes

		@@ -330,7 +330,7 @@ func GetComponentSource(client *occlient.Client, componentName string, applicati
		// Component name is the name component to be updated
		// to indicates what type of source type the component source is changing to e.g from git to local or local to binary


		var _ = Describe("updateE2e", func() {

		bootStrapSupervisorURI := "https://github.com/kadel/bootstrap-supervisored-s2i"

Fixes odo update from git to binary/local and vice versa #518

Fixes odo update from git to binary/local and vice versa #518

Conversation

mik-dass commented Jun 7, 2018 • edited by cdrage Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel left a comment

Choose a reason for hiding this comment

mik-dass commented Jun 12, 2018

kadel commented Jun 12, 2018

kadel commented Jun 12, 2018

mik-dass commented Jun 12, 2018

kadel commented Jun 12, 2018 • edited Loading

mik-dass commented Jun 20, 2018

kadel 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadel Jun 26, 2018 • 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

cdrage commented Jul 16, 2018

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

mik-dass commented Jul 18, 2018

kadel 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

mik-dass commented Jul 19, 2018

cdrage commented Jul 19, 2018

kadel left a comment • edited Loading

Choose a reason for hiding this comment

cdrage commented Jul 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mik-dass commented Jul 26, 2018

mik-dass commented Jun 7, 2018 •

edited by cdrage

Loading

kadel commented Jun 12, 2018 •

edited

Loading

kadel Jun 26, 2018 •

edited

Loading

kadel left a comment •

edited

Loading