kill: adds support for numeric signals #107

devimc · 2017-05-03T13:52:39Z

With this commit kill command is able to handle numeric
signals as argument

Signed-off-by: Julio Montes julio.montes@intel.com

jodh-intel · 2017-05-03T14:07:11Z

kill.go

+		for _, v := range signals {
+			if v == signum {
+				// signal is a valid signal
+				if err := vc.KillContainer(containerID, podStatus.ContainersStatus[0].ID, signum); err != nil {


I'd prefer only a single call to vc.KillContainer() if possible.

Also, note #109 if you want to pick that up?

yep, I'll take this issue

sboeuf

The implementation seems good to me but I have two requests:

Could you please create a separate function doing this processing about the signal:

func processSignal(signal string) (syscall.Signal, error)

and then call it from the current kill() function so that you actually only add

processedSignal, err := processSignal(signal)
if err != nil {
        return err
}

before the call to vc.KillContainer()

And could you add the corresponding unit test to the function you've added.

dlespiau · 2017-05-03T14:19:42Z

kill.go

 	}

-	if err := vc.KillContainer(containerID, podStatus.ContainersStatus[0].ID, signals[signal]); err != nil {
+	signum, signalOk := signals[signal]


You have the same code repeated twice now with some somewhat deep nested indentation.

if err := vc.KillContainer(containerID, podStatus.ContainersStatus[0].ID, signum); err != nil { return err } return nil

resolve the argument to get a signal number

do the KillContainer call

For 1. we don't actually have to cycle through the map, we can just test if the signal is between 1 and 31 (or SIGHUP and SIGUNUSED).

devimc · 2017-05-03T14:57:24Z

@jodh-intel @sboeuf @dlespiau Fixed
Thanks

coveralls · 2017-05-03T15:02:52Z

Coverage increased (+1.5%) to 37.192% when pulling 468bebe on devimc:topic/fix_kill into 6c019a5 on clearcontainers:master.

coveralls · 2017-05-03T15:07:07Z

Coverage increased (+1.5%) to 37.192% when pulling 468bebe on devimc:topic/fix_kill into 6c019a5 on clearcontainers:master.

coveralls · 2017-05-03T15:07:07Z

Coverage increased (+1.5%) to 37.192% when pulling 468bebe on devimc:topic/fix_kill into 6c019a5 on clearcontainers:master.

dlespiau · 2017-05-03T15:08:59Z

kill_test.go

+	"testing"
+)
+
+func TestProcessSignal(t *testing.T) {


The usual thing here to limit the code to type and maintain is to use list the test cases in an array of structs (often using anonymous structs): https://dave.cheney.net/2013/06/09/writing-table-driven-tests-in-go

something like

tests := []struct { input string valid bool golden syscall.Signal } { /* list of test cases */ } for _, test := range tests { ..... }

An example at: https://github.com/dlespiau/x86db/blob/master/db_test.go#L10

Or another way is to actually create each test such as TestProcessSignalInvalidSignal(), TestProcessSignalInvalidShortSignal(), ... into a generic function like testProcessSignalFailure() or testProcessSignalSuccess().
That way each unit test has a name and it is very quick to get which test failed.

@sboeuf that adds quite a lot of typing and duplication actually. If the input needs some description, it can be added to the entries of the table, especially for very simple functions like that one that just take a string as input and return an int.

dlespiau · 2017-05-03T15:20:21Z

with the list of test cases input in a struct this is:

lgtm

sboeuf

Few changes needed, but this is much better :)

sboeuf · 2017-05-03T15:31:52Z

kill.go

+	s, err := strconv.Atoi(signal)
+	if err != nil {
+		return 0, fmt.Errorf("Failed to convert signal %s to int", signal)
+	}


Add a new line please.

sboeuf · 2017-05-03T15:33:05Z

kill.go

+		return 0, fmt.Errorf("Failed to convert signal %s to int", signal)
+	}
+	signum = syscall.Signal(s)
+	// check whether signal is valid or not


s/check/Check/

sboeuf · 2017-05-03T15:33:20Z

kill.go

+			// signal is a valid signal
+			return signum, nil
+		}
+	}


Add a new line.

sboeuf · 2017-05-03T15:36:13Z

kill.go

+	}
+	signum = syscall.Signal(s)
+	// check whether signal is valid or not
+	for _, v := range signals {


Maybe use "sig" instead of "v" because this will be easier to understand.

sboeuf · 2017-05-03T15:41:31Z

kill_test.go

+	"testing"
+)
+
+func TestProcessSignal(t *testing.T) {


Or another way is to actually create each test such as TestProcessSignalInvalidSignal(), TestProcessSignalInvalidShortSignal(), ... into a generic function like testProcessSignalFailure() or testProcessSignalSuccess().
That way each unit test has a name and it is very quick to get which test failed.

sboeuf · 2017-05-03T15:44:49Z

Oh BTW @devimc could you please rebase recent changes from github.com/clearcontainers/runtime:origin/master so that Semaphore CI build can pass.

dlespiau · 2017-05-03T15:49:13Z

kill_test.go

+	}
+
+	for _, test := range tests {
+		signum, _ := processSignal(test.signal)


you're missing testing we receive a non nil error on invalid input.

coveralls · 2017-05-03T15:52:16Z

Coverage increased (+1.5%) to 37.192% when pulling 317828d on devimc:topic/fix_kill into 6c019a5 on clearcontainers:master.

sboeuf

After @dlespiau comment is addressed, this lgtm

coveralls · 2017-05-03T16:31:45Z

Coverage increased (+1.5%) to 37.192% when pulling 6f51ef2 on devimc:topic/fix_kill into 6c019a5 on clearcontainers:master.

devimc · 2017-05-03T16:32:30Z

@dlespiau @sboeuf fixed

With this commit kill command is able to handle numeric signals as argument Fixes clearcontainers#109 Signed-off-by: Julio Montes <julio.montes@intel.com>

dlespiau · 2017-05-03T17:14:57Z

\o/

coveralls · 2017-05-03T17:15:17Z

Coverage increased (+1.5%) to 37.192% when pulling 08db9ff on devimc:topic/fix_kill into 79541cd on clearcontainers:master.

devimc requested review from jodh-intel and sboeuf May 3, 2017 13:53

jodh-intel reviewed May 3, 2017

View reviewed changes

sboeuf suggested changes May 3, 2017

View reviewed changes

dlespiau reviewed May 3, 2017

View reviewed changes

devimc force-pushed the topic/fix_kill branch 2 times, most recently from 897762b to 468bebe Compare May 3, 2017 14:56

dlespiau reviewed May 3, 2017

View reviewed changes

sboeuf suggested changes May 3, 2017

View reviewed changes

devimc force-pushed the topic/fix_kill branch from 468bebe to 317828d Compare May 3, 2017 15:47

dlespiau reviewed May 3, 2017

View reviewed changes

sboeuf approved these changes May 3, 2017

View reviewed changes

devimc force-pushed the topic/fix_kill branch from 317828d to 6f51ef2 Compare May 3, 2017 16:27

kill: adds support for numeric signals

08db9ff

With this commit kill command is able to handle numeric signals as argument Fixes clearcontainers#109 Signed-off-by: Julio Montes <julio.montes@intel.com>

devimc force-pushed the topic/fix_kill branch from 6f51ef2 to 08db9ff Compare May 3, 2017 17:09

sboeuf merged commit a246d4e into clearcontainers:master May 3, 2017

devimc deleted the topic/fix_kill branch May 24, 2017 14:00

kill: adds support for numeric signals #107

kill: adds support for numeric signals #107

Uh oh!

Conversation

devimc commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sboeuf left a comment • 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!

dlespiau May 3, 2017 • 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!

devimc commented May 3, 2017

Uh oh!

coveralls commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented May 3, 2017

Uh oh!

coveralls commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlespiau May 3, 2017 • 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!

dlespiau May 3, 2017 • 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!

dlespiau commented May 3, 2017 • edited by sameo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sboeuf 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!

sboeuf commented May 3, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sboeuf left a comment

Choose a reason for hiding this comment

Uh oh!

coveralls commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devimc commented May 3, 2017

Uh oh!

dlespiau commented May 3, 2017

Uh oh!

coveralls commented May 3, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

sboeuf left a comment •

edited

Loading

dlespiau May 3, 2017 •

edited

Loading

coveralls commented May 3, 2017 •

edited

Loading

coveralls commented May 3, 2017 •

edited

Loading

dlespiau May 3, 2017 •

edited

Loading

dlespiau May 3, 2017 •

edited

Loading

dlespiau commented May 3, 2017 •

edited by sameo

Loading

coveralls commented May 3, 2017 •

edited

Loading

coveralls commented May 3, 2017 •

edited

Loading

coveralls commented May 3, 2017 •

edited

Loading