Add information about ports to `odo create` output #610

anmolbabu · 2018-08-01T09:09:46Z

This PR adds message of the form:
"Component $componentName was created and ports $containerPort/$containerProtocol,.... were opened."

fixes #608
Signed-off-by: anmolbabu [email protected]

anmolbabu · 2018-08-01T10:04:40Z

Will change fmt.Print statements to glog statements once #590 is in

kadel · 2018-08-01T10:35:10Z

Will change fmt.Print statements to glog statements once #590 is in

user messages like this should remain fmt.Print

kadel · 2018-08-01T10:38:37Z

pkg/component/component.go

@@ -68,6 +68,31 @@ func CreateFromGit(client *occlient.Client, name string, ctype string, url strin
 	return nil
 }

+// GetComponentPorts provides map[container_name][]array_of_ports
+func GetComponentPorts(client *occlient.Client, componentName string, applicationName string, additional bool) (res []string, err error) {


additional argument is never used

Aah my bad :(

kadel · 2018-08-01T10:41:21Z

pkg/component/component.go

@@ -68,6 +68,31 @@ func CreateFromGit(client *occlient.Client, name string, ctype string, url strin
 	return nil
 }

+// GetComponentPorts provides map[container_name][]array_of_ports


the comment says that provides map but this function returns a slice of strings

Will fix it to slice.
Forgot to modify comment post changing code

kadel · 2018-08-01T10:44:39Z

pkg/component/component.go

+
+	dc, err := client.GetOneDeploymentConfigFromSelector(componentSelector)
+	if err != nil {
+		return nil, errors.Wrapf(err, "Unable to fetch deployment configs for the selector %v", componentSelector)


Go errors shouldn't start with a capital letter. - https://github.com/golang/go/wiki/Errors

kadel · 2018-08-01T10:51:31Z

pkg/component/component.go

+	// Wait for Pod to be in running state otherwise we can't sync data to it.
+	pod, err := client.WaitAndGetPod(podSelector)
+	if err != nil {
+		return nil, errors.Wrapf(err, "Error fetching pod for podselector %v", podSelector)


Go errors shouldn't start with a capital letter. - https://github.com/golang/go/wiki/Errors

kadel · 2018-08-01T10:53:50Z

odo-bug-id: #608

Please use "fixes #608" GitHub will automatically close that issue when we merge this PR. Otherwise, it is easy to forget to close the issue.

anmolbabu · 2018-08-01T11:04:41Z

@kadel Done incorporated your comments
Can you please revisit the PR

kadel · 2018-08-01T11:26:38Z

cmd/create.go

 		}
+		ports, err := component.GetComponentPorts(client, componentName, applicationName, false)


it doesn't really make sense to call this function with additional flag set to true.
You can just completely remove this argument from the function and always call componentlabels.GetLabels with false. Sorry this was what I meant with my previous comment 😇

oh ok np :)

anmolbabu · 2018-08-01T11:43:54Z

@kadel Incorporated your comments
Please review

!! Verified !!

anmolbabu · 2018-08-01T11:46:27Z

@cdrage @surajnarwade @syamgk @mik-dass Please review

cdrage

Needs tests!

cdrage · 2018-08-01T18:32:30Z

cmd/create.go

+		} else if len(ports) == 1 {
+			fmt.Printf(" and port %s was opened\n", ports[0])
+		}
+		fmt.Printf("To push source code to the component run 'odo push'\n")


The code above is a bit too cluttered. Please add newlines / spacing between the different parts of the code. For example:

ports, err := component.GetComponentPorts(client, componentName, applicationName) checkError(err, "") fmt.Printf("Component '%s' was created", componentName) if len(ports) > 1 { fmt.Printf(" and ports %s were opened\n", strings.Join(ports, ",")) } else if len(ports) == 1 { fmt.Printf(" and port %s was opened\n", ports[0]) } fmt.Printf("To push source code to the component run 'odo push'\n"

This helps ensure clarity in the code when reading

cdrage · 2018-08-01T18:33:24Z

pkg/component/component.go

+	if err != nil {
+		return nil, errors.Wrapf(err, "unable to fetch deployment configs for the selector %v", componentSelector)
+	}
+	// Find Pod for component


newline above

cdrage · 2018-08-01T18:33:29Z

pkg/component/component.go

+	}
+	// Find Pod for component
+	podSelector := fmt.Sprintf("deploymentconfig=%s", dc.Name)
+	// Wait for Pod to be in running state otherwise we can't sync data to it.


newline above

cdrage · 2018-08-01T18:33:33Z

pkg/component/component.go

+	if err != nil {
+		return nil, errors.Wrapf(err, "error fetching pod for podselector %v", podSelector)
+	}
+	//Iterate and fetch ports


newline above

cdrage · 2018-08-01T18:35:43Z

pkg/component/component.go

@@ -68,6 +68,31 @@ func CreateFromGit(client *occlient.Client, name string, ctype string, url strin
 	return nil
 }

+// GetComponentPorts provides slice of ports used by the component in the form port_no/protocol
+func GetComponentPorts(client *occlient.Client, componentName string, applicationName string) (res []string, err error) {


Hate to say this, but you'll need to add a test for this function 😄

If you need help, feel free to PM / ping me or @ashetty1 / @mik-dass ! I just finished doing #565 and adding tests there and had to learn a bunch of new stuff.

cdrage · 2018-08-01T18:38:15Z

pkg/component/component.go

@@ -68,6 +68,31 @@ func CreateFromGit(client *occlient.Client, name string, ctype string, url strin
 	return nil
 }

+// GetComponentPorts provides slice of ports used by the component in the form port_no/protocol
+func GetComponentPorts(client *occlient.Client, componentName string, applicationName string) (res []string, err error) {


a different variable name than res would be better.

Maybe ports ?

mik-dass · 2018-08-02T09:54:36Z

pkg/component/component.go

+	// Find Pod for component
+	podSelector := fmt.Sprintf("deploymentconfig=%s", dc.Name)
+	// Wait for Pod to be in running state otherwise we can't sync data to it.
+	pod, err := client.WaitAndGetPod(podSelector)


I guess we can get the containers and thus the ports from dc.Spec.Template.Spec.Containers instead of retrieving the Pod

anmolbabu · 2018-08-04T08:00:46Z

Verified as in https://gist.github.com/anmolbabu/fb066e37739750064c49797eeae8c00c
@kadel @cdrage @surajnarwade @syamgk @mik-dass Incorporated your comments.
Please review

surajnarwade · 2018-08-06T09:26:39Z

cmd/create.go

+			fmt.Printf(" and port %s was opened\n", ports[0])
+		}
+
+		fmt.Printf("To push source code to the component run 'odo push'\n")


we don't need this line if our source is git

surajnarwade · 2018-08-06T09:28:37Z

pkg/testingutil/deploymentconfigs.go

+	}
+}
+
+/*


Remove comments

anmolbabu · 2018-08-06T16:45:42Z

@surajnarwade Incoporated your comments Please revisit the PR

@kadel @cdrage @surajnarwade @syamgk @mik-dass Please review

anmolbabu · 2018-08-07T07:30:37Z

Verified as in : https://gist.github.com/anmolbabu/7a908af61cb46e633471eebe370a37db

cdrage

@mik-dass and @ashetty1 please review the tests as you two are the review masterssss!

just a quick question from me @anmolbabu but this looks great so far!

cdrage · 2018-08-07T15:12:20Z

pkg/component/component_test.go

+			// Fake the client with the appropriate arguments
+			client, fakeClientSet := occlient.FakeNew()
+			fakeClientSet.AppsClientset.PrependReactor("list", "deploymentconfigs", func(action ktesting.Action) (bool, runtime.Object, error) {
+				//return true, testingutil.FakeDeploymentConfigs(tt.args.namespace, tt.args.componentName, tt.args.componentType, tt.args.applicationName, tt.args.containerPort), nil


why aren't we using this one instead (generating the deploymentmentconfigs with passed in params)?

only to avoid long list of parameters and multiple calls like if you see https://github.com/redhat-developer/odo/pull/610/files#diff-d89c4df0f7eae3a323f4958758b13435R28,

In some cases I have 2 containers per dc in some 1 and within a container, I have in some cases 2 ports exposed and in some, 1 which means the private functions in that file will also need to be exported. In fact the huge list of parameters vs long list of function calls made me feel would clutter the code here so felt it would be nice to keep it in the file where mock data is present :)

okay. can you remove this line @anmolbabu since it's commented out?

cdrage · 2018-08-07T15:12:30Z

pkg/component/component_test.go

+			// The function we are testing
+			output, err := GetComponentPorts(client, tt.args.componentName, tt.args.applicationName)
+
+			//Checks for error in positive cases


add spacing after //

mik-dass

LGTM :)

kadel · 2018-08-08T12:33:28Z

@cdrage @surajnarwade please check if your comments were addressed so we can merge it

This PR adds message of the form: "Component $componentName was created and ports $containerPort/$containerProtocol,.... were opened." fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Signed-off-by: anmolbabu <[email protected]>

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

cdrage

Please remove the commented out code. Other than that, LGTM.

cdrage · 2018-08-08T14:16:13Z

pkg/component/component_test.go

+			// Fake the client with the appropriate arguments
+			client, fakeClientSet := occlient.FakeNew()
+			fakeClientSet.AppsClientset.PrependReactor("list", "deploymentconfigs", func(action ktesting.Action) (bool, runtime.Object, error) {
+				//return true, testingutil.FakeDeploymentConfigs(tt.args.namespace, tt.args.componentName, tt.args.componentType, tt.args.applicationName, tt.args.containerPort), nil


okay. can you remove this line @anmolbabu since it's commented out?

anmolbabu · 2018-08-08T15:59:43Z

@cdrage Done.Can you push this in if looks good?

cdrage · 2018-08-08T16:15:23Z

Tests were a false-positive. I'm re-running them to see if it passes and then we'll merge!

surajnarwade · 2018-08-09T05:35:04Z

Travis status is not green, I am retriggering the build. if it get passed, I'll merge it :)

anmolbabu 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 Aug 1, 2018

anmolbabu self-assigned this Aug 1, 2018

anmolbabu force-pushed the issue#608 branch from 320e28c to 18fb90c Compare August 1, 2018 10:01

anmolbabu requested review from kadel, cdrage, syamgk, mik-dass and surajnarwade August 1, 2018 10:03

anmolbabu force-pushed the issue#608 branch from 18fb90c to 8834174 Compare August 1, 2018 10:21

anmolbabu removed 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 Aug 1, 2018

kadel reviewed Aug 1, 2018

View reviewed changes

anmolbabu force-pushed the issue#608 branch from 8834174 to 54fd375 Compare August 1, 2018 11:03

kadel reviewed Aug 1, 2018

View reviewed changes

cdrage suggested changes Aug 1, 2018

View reviewed changes

mik-dass reviewed Aug 2, 2018

View reviewed changes

anmolbabu force-pushed the issue#608 branch from f5a87ee to 554465a Compare August 4, 2018 07:58

surajnarwade suggested changes Aug 6, 2018

View reviewed changes

surajnarwade reviewed Aug 6, 2018

View reviewed changes

anmolbabu force-pushed the issue#608 branch from e7c5f04 to cbe302a Compare August 7, 2018 04:05

cdrage suggested changes Aug 7, 2018

View reviewed changes

kadel approved these changes Aug 8, 2018

View reviewed changes

mik-dass approved these changes Aug 8, 2018

View reviewed changes

anmolbabu added 10 commits August 8, 2018 18:16

Add information about ports to odo create output

439b228

This PR adds message of the form: "Component $componentName was created and ports $containerPort/$containerProtocol,.... were opened." fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Incoporate comments by @tkral

75f14de

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Fixes interleaving of waitandgetpod b/w ports msg and component created

56d7eda

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Incorporate @tkral comments

19d8085

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Incoporate @cdrage comments

b9b8e4f

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Incorporate @cdrage comments

5726bc1

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Incoporate @mik-das comments

e8014d0

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Add additional tests

83f6a90

Signed-off-by: anmolbabu <[email protected]>

Incoporate comments by @snarwade

95b12dc

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

Add space in comments

7fbf4f1

fixes redhat-developer#608 Signed-off-by: anmolbabu <[email protected]>

anmolbabu force-pushed the issue#608 branch from de4a5ad to 7fbf4f1 Compare August 8, 2018 12:54

cdrage suggested changes Aug 8, 2018

View reviewed changes

Remove commented out line

b4789b6

cdrage approved these changes Aug 8, 2018

View reviewed changes

surajnarwade approved these changes Aug 9, 2018

View reviewed changes

surajnarwade merged commit f880cc2 into redhat-developer:master Aug 9, 2018

surajnarwade mentioned this pull request Aug 24, 2018

v0.10.0 #661

Merged

		}
		ports, err := component.GetComponentPorts(client, componentName, applicationName, false)

+              	}
+              }
+              /*

Add information about ports to odo create output #610

Add information about ports to odo create output #610

Uh oh!

Conversation

anmolbabu commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmolbabu commented Aug 1, 2018

Uh oh!

kadel commented Aug 1, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolbabu Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kadel commented Aug 1, 2018

Uh oh!

anmolbabu commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolbabu commented Aug 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anmolbabu commented Aug 1, 2018

Uh oh!

cdrage left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mik-dass Aug 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anmolbabu commented Aug 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Add information about ports to `odo create` output #610

Add information about ports to `odo create` output #610

anmolbabu commented Aug 1, 2018 •

edited

Loading

anmolbabu Aug 1, 2018 •

edited

Loading

anmolbabu commented Aug 1, 2018 •

edited

Loading

anmolbabu commented Aug 1, 2018 •

edited

Loading

mik-dass Aug 2, 2018 •

edited

Loading

anmolbabu commented Aug 4, 2018 •

edited

Loading