Bug 1595153 - Add support for apb test that works in and out of cluster #109

dymurray · 2018-08-08T17:05:36Z

In the past we were checking the contents of a file on the container to check if the APB succeeded in the test. This seemed incredibly pointless to me so instead I have talked with @djzager and determined we should simply judge a test status on whether or not the container is in a failed state or a succeeded state. This makes the workflow much simpler and now if a person wants to write a set of unit tests they can simply run the fail module in Ansible to signify a failed test which will put the pod in an error state.

djwhatle · 2018-08-08T20:22:42Z

pkg/runner/runner.go

@@ -40,7 +40,7 @@ import (
 )

 // RunBundle will run the bundle's action in the given namespace
-func RunBundle(action string, ns string, bundleName string, sandboxRole string, bundleRegistry string, printLogs bool, skipParams bool, args []string) {
+func RunBundle(action string, ns string, bundleName string, sandboxRole string, bundleRegistry string, printLogs bool, skipParams bool, args []string) (string, error) {


I may be missing something, but I'm having trouble figuring out why we're returning a string here. Looks like it's empty in all return paths below.

Perhaps using named return values would be a good idea if you want to use the string in the future, might make it less ambiguous.

Figured it out below, think we should name the string podName

djwhatle · 2018-08-08T20:23:53Z

cmd/bundle.go

@@ -208,16 +238,37 @@ func ListImages() {
 	}
 }

-func executeBundle(action string, args []string) {
+func executeBundle(action string, args []string) string {


Same thing here, what's this return value for?

Edit: I see now, looks like the pod name. Maybe we should name it though. I'll let you decide.

djwhatle · 2018-08-08T20:26:33Z

pkg/runner/runner.go

@@ -60,15 +60,15 @@ func RunBundle(action string, ns string, bundleName string, sandboxRole string,
 	if len(candidateSpecs) == 0 {
 		if len(bundleRegistry) > 0 {
 			log.Errorf("Didn't find APB [%v] in registry [%v]\n", bundleName, bundleRegistry)
-			return
+			return "", errors.New(fmt.Sprintf("failed to find APB [%v] in registry [%v]", bundleName, bundleRegistry))


I think we're going to end up with one too many error messages being printed here.

ERR - Didn't find APB [bar] in registry foo ERR - Failed to execute bundle [bar]: [failed to find APB [bar] in registry [foo]]

^ Above will be the output from what I can tell. Seems redundant to print info about not being able to find the APB multiple times.

djwhatle · 2018-08-08T20:30:55Z

pkg/runner/runner.go

 		}
 		log.Errorf("Didn't find APB [%v] in configured registries\n", bundleName)
-		return
+		return "", errors.New(fmt.Sprintf("failed to find APB [%v] on configured registries", bundleName))


Same thing here

djwhatle · 2018-08-08T20:31:01Z

pkg/runner/runner.go

 		// TODO: return an ErrorBundleNotFound
 	}
 	if len(candidateSpecs) > 1 {
 		log.Warnf("Found multiple APBs matching name [%v]. Specify a registry with --registry.", bundleName)
-		return
+		return "", errors.New(fmt.Sprintf("found multiple APBs with matching name [%v]", bundleName))


djwhatle · 2018-08-08T20:32:44Z

pkg/runner/runner.go

@@ -169,7 +169,21 @@ func RunBundle(action string, ns string, bundleName string, sandboxRole string,
 	}

 	// TODO: return nil
-	return
+	return pn, nil


Ah, now I see it. I think it would be helpful if we named the return value since the function name doesn't explicitly tell us what kind of return value to expect in the string.

at a minimum, I think if you are going to use the named return the names should match.

djwhatle

Think we could make these functions more readable by naming the new return values. I don't have a working cluster to test "apb test" with right now, but that part looks good to me visually.

Also think our error printing is a little weird right now.

Thanks Derek! Made the requested changes

djzager

Looks good, minor nits.

djzager · 2018-08-09T21:17:40Z

cmd/bundle.go

 		}
 	}
 	log.Debugf("Running bundle [%v] with action [%v] in namespace [%v].", args[0], action, bundleNamespace)
-	runner.RunBundle(action, bundleNamespace, args[0], sandboxRole, bundleRegistry, printLogs, skipParams, args[1:])
+	pn, err := runner.RunBundle(action, bundleNamespace, args[0], sandboxRole, bundleRegistry, printLogs, skipParams, args[1:])


my understanding of how to use "named returns" is based on https://tour.golang.org/basics/7.

Based on that I would think that you should initialize podName in the beginning to an empty string, set it here, and simply use return. I think that way would be much easier to read.

Wow I had no idea you could do this. Thanks!

While the named return is an interesting thing, I would NOT use it in this instance. I think it would be more confusing since then you'd have to look at the empty return, then look at the method signature to see if it is a named return or not. If this were a much SHORTER method then I'd be okay with it. But I would leave it as return pn

djzager · 2018-08-09T21:20:57Z

pkg/runner/runner.go

@@ -169,7 +169,21 @@ func RunBundle(action string, ns string, bundleName string, sandboxRole string,
 	}

 	// TODO: return nil
-	return
+	return pn, nil


at a minimum, I think if you are going to use the named return the names should match.

jmrodri

the initialization of podName and err is weird, much more pythonlike than golike.

jmrodri · 2018-08-14T13:57:42Z

cmd/bundle.go

 		}
 	}
 	log.Debugf("Running bundle [%v] with action [%v] in namespace [%v].", args[0], action, bundleNamespace)
-	runner.RunBundle(action, bundleNamespace, args[0], sandboxRole, bundleRegistry, printLogs, skipParams, args[1:])
+	pn, err := runner.RunBundle(action, bundleNamespace, args[0], sandboxRole, bundleRegistry, printLogs, skipParams, args[1:])


While the named return is an interesting thing, I would NOT use it in this instance. I think it would be more confusing since then you'd have to look at the empty return, then look at the method signature to see if it is a named return or not. If this were a much SHORTER method then I'd be okay with it. But I would leave it as return pn

jmrodri · 2018-08-14T13:58:38Z

pkg/runner/runner.go

-func RunBundle(action string, ns string, bundleName string, sandboxRole string, bundleRegistry string, printLogs bool, skipParams bool, args []string) {
+func RunBundle(action string, ns string, bundleName string, sandboxRole string, bundleRegistry string, printLogs bool, skipParams bool, args []string) (podName string, err error) {
+	podName = ""
+	err = nil


this should be:

var podName string var err error

They will get the default values of "" and nil respectively.

jmrodri · 2018-08-14T14:00:35Z

pkg/runner/runner.go

+	pod, err := k8scli.Client.CoreV1().Pods(namespace).Get(podName, metav1.GetOptions{})
+	if err != nil {
+		log.Errorf("Error getting pod [%v] status: %v", podName, err)
+		return ""


feels like this should return something other than empty string.

Going to include an error return as well

djwhatle reviewed Aug 8, 2018

View reviewed changes

djwhatle previously requested changes Aug 8, 2018

View reviewed changes

djzager approved these changes Aug 9, 2018

View reviewed changes

jmrodri requested changes Aug 14, 2018

View reviewed changes

dymurray added 4 commits August 14, 2018 11:02

Bug 1595153 - Add support for apb test that works in and out of cluster

89ddcb1

Feedback updates

3fe365a

Update named returns

ec42710

Feedback updates

ff443fa

dymurray force-pushed the apbtest branch from 0ca5bef to ff443fa Compare August 14, 2018 15:34

jmrodri approved these changes Aug 14, 2018

View reviewed changes

dymurray merged commit edbe861 into automationbroker:master Aug 14, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1595153 - Add support for apb test that works in and out of cluster #109

Bug 1595153 - Add support for apb test that works in and out of cluster #109

dymurray commented Aug 8, 2018 •

edited

djwhatle Aug 8, 2018 •

edited

djwhatle Aug 8, 2018

djwhatle Aug 8, 2018 •

edited

djwhatle Aug 8, 2018

djwhatle Aug 8, 2018

djwhatle Aug 8, 2018

djwhatle Aug 8, 2018

djwhatle Aug 8, 2018

djzager Aug 9, 2018

djwhatle left a comment •

edited

djzager left a comment

djzager Aug 9, 2018

dymurray Aug 10, 2018

jmrodri Aug 14, 2018

djzager Aug 9, 2018

jmrodri left a comment

jmrodri Aug 14, 2018

jmrodri Aug 14, 2018

jmrodri Aug 14, 2018

dymurray Aug 14, 2018

Bug 1595153 - Add support for apb test that works in and out of cluster #109

Bug 1595153 - Add support for apb test that works in and out of cluster #109

Conversation

dymurray commented Aug 8, 2018 • edited

djwhatle Aug 8, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djwhatle Aug 8, 2018 • edited

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

djwhatle left a comment • edited

Choose a reason for hiding this comment

djzager 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

jmrodri 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

dymurray commented Aug 8, 2018 •

edited

djwhatle Aug 8, 2018 •

edited

djwhatle Aug 8, 2018 •

edited

djwhatle left a comment •

edited