Enable setting environment variables via Hooks #27

AlfredBroda · 2017-11-30T13:53:40Z

This allows for setting application environment via Hooks (for remote configuration retrieval purposes).

coveralls · 2017-11-30T13:57:29Z

Coverage increased (+0.5%) to 75.92% when pulling 2640253 on feature_set_env into 1648fdf on master.

coveralls · 2017-11-30T14:03:13Z

Coverage increased (+0.5%) to 75.92% when pulling a3a0329 on feature_set_env into 1648fdf on master.

medzin

It will not work - hook recives copy of TaskInfo. You have to modify Hook interface so it will allow to return values.

coveralls · 2017-11-30T14:20:35Z

Coverage increased (+0.4%) to 75.79% when pulling aae731b on feature_set_env into 6705a3a on master.

janisz

Change commit message. You add only test now.

coveralls · 2017-11-30T14:50:25Z

Coverage increased (+0.4%) to 75.79% when pulling 42947c0 on feature_set_env into 6705a3a on master.

medzin · 2017-12-01T12:32:37Z

@AlfredBroda why I see "Fix Travis configuration for releses #22" in this PR?

coveralls · 2017-12-01T12:52:46Z

Coverage increased (+0.4%) to 74.945% when pulling 816e858 on feature_set_env into 653f93c on master.

AlfredBroda · 2017-12-01T13:30:24Z

Fixed all the conflicts... for now.

coveralls · 2017-12-01T13:37:25Z

Coverage increased (+1.6%) to 76.139% when pulling bd84cbe on feature_set_env into 653f93c on master.

janisz · 2017-12-01T13:36:58Z

command.go

@@ -121,7 +122,7 @@ func (c *cancellableCommand) Stop(gracePeriod time.Duration) {
 }

 // NewCommand returns a new command based on passed CommandInfo.
-func NewCommand(commandInfo mesos.CommandInfo, env []string, options ...func(*exec.Cmd) error) (Command, error) {
+func NewCommand(commandInfo mesos.CommandInfo, env hook.Env, options ...func(*exec.Cmd) error) (Command, error) {


Env should not be defined in hook package

Where do you suggest to put it? executor.go? command.go?

Reverted back to []string

janisz · 2017-12-01T13:38:10Z

hook/consul/hook.go

 	default:
 		log.Debugf("Received unsupported event type %s - ignoring", event.Type)
-		return nil // ignore unsupported events
+		return hook.Env{}, nil // ignore unsupported events


IMO nil will be better than hook.Env{}

It's not a problem really - you have to perform error check before you will use that value.

And deal with mocking in tests. Will try that approach (again).

I mean, it does not make sense to return pointer here.

Returning nil then.

medzin · 2017-12-01T13:53:54Z

executor.go

+		Type:     hook.BeforeTaskStartEvent,
+		TaskInfo: mesosutils.TaskInfo{TaskInfo: taskInfo},
+	}
+	additionalEnv, err := e.hookManager.HandleEvent(beforeStartEvent, false)


You have to merge env and additionalEnv slices or services will not have access to certificate or MESOS_ variables.

Also add test case for this - the current ones did not catch this problem (os.Environ() is not copied into command env)

medzin

Correct the submitted comments.

coveralls · 2017-12-01T14:32:24Z

Coverage increased (+1.0%) to 75.56% when pulling 7b2fad2 on feature_set_env into 653f93c on master.

medzin · 2017-12-01T14:32:42Z

@AlfredBroda explain in commit message why we are enabling this.

medzin · 2017-12-01T14:40:40Z

hook/hook.go

@@ -25,12 +25,15 @@ type Event struct {
 	TaskInfo mesosutils.TaskInfo
 }

+// Env is a container type for environment variables.


Add info about format. Most people will expect map here - not slice.

medzin · 2017-12-01T14:42:18Z

mesosutils/taskinfo_test.go

+
+	returnedValue := taskInfo.FindEnvValue(key)
+
+	require.Equal(t, returnedValue, expectedValue)


Signature of Equal is:

func Equal(t TestingT, expected interface{}, actual interface{}, msgAndArgs ...interface{})

(switch returnedValue and expectedValue)

coveralls · 2017-12-01T14:52:45Z

Coverage increased (+1.5%) to 75.771% when pulling 910c347 on feature_set_env into a0fa859 on master.

coveralls · 2017-12-01T15:00:56Z

Coverage increased (+1.5%) to 75.771% when pulling 910c347 on feature_set_env into a0fa859 on master.

coveralls · 2017-12-04T09:47:07Z

Coverage increased (+1.4%) to 75.627% when pulling de3e296 on feature_set_env into a0fa859 on master.

coveralls · 2017-12-04T12:37:48Z

Coverage increased (+1.5%) to 75.46% when pulling 2474a3b on feature_set_env into 36a4c04 on master.

Fixes done, no time to wait for your ok. :P

janisz

We could make use of named return values. E.g., when it's alywas nil we can call it ignore.

I'd like to make code cleanup in separated commits.

janisz · 2017-12-04T20:14:41Z

executor.go

@@ -405,8 +414,8 @@ func (e *Executor) shutDown(taskInfo mesos.TaskInfo, cmd Command) {
 		Type:     hook.BeforeTerminateEvent,
 		TaskInfo: mesosutils.TaskInfo{TaskInfo: taskInfo},
 	}
-	_ = e.hookManager.HandleEvent(beforeTerminateEvent, true) // ignore errors here, so every hook will have a chance to be called
-	cmd.Stop(gracePeriod)                                     // blocking call
+	_, _ = e.hookManager.HandleEvent(beforeTerminateEvent, true) // ignore errors here, so every hook will have a chance to be called


I think we should log errors

janisz · 2017-12-04T20:15:19Z

hook/consul/hook.go

@@ -46,15 +46,15 @@ type Config struct {

 // HandleEvent calls appropriate hook functions that correspond to supported
 // event types. Unsupported events are ignored.
-func (h *Hook) HandleEvent(event hook.Event) error {
+func (h *Hook) HandleEvent(event hook.Event) (hook.Env, error) {


Please document when HandleEvent returns hook.Env

janisz · 2017-12-04T20:15:54Z

hook/hook.go

@@ -25,12 +25,15 @@ type Event struct {
 	TaskInfo mesosutils.TaskInfo
 }

+// Env is a container for os.Environ style list of combined environment variable strings.
+type Env []string


I think this should not be in this package

Suggest another then 😄

janisz · 2017-12-04T20:16:19Z

hook/hook.go

 // Hook is an interface for various executor extensions, that can add some actions
 // during task lifecycle events.
 type Hook interface {
 	// HandleEvent is called when any of defined executor task events occurs.
 	// Received events may be handled in any way, but hook should ignore unknown or
 	// unsupported ones. Call to this function will block executor process until
 	// it returns. Order of received event types is undefined.
-	HandleEvent(Event) error
+	HandleEvent(Event) (Env, error)


Update the godoc

AlfredBroda added the enhancement label Nov 30, 2017

AlfredBroda self-assigned this Nov 30, 2017

AlfredBroda requested review from medzin and janisz November 30, 2017 13:53

AlfredBroda force-pushed the feature_set_env branch from 2640253 to a3a0329 Compare November 30, 2017 13:58

janisz approved these changes Nov 30, 2017

View reviewed changes

medzin suggested changes Nov 30, 2017

View reviewed changes

AlfredBroda force-pushed the feature_set_env branch from a3a0329 to aae731b Compare November 30, 2017 14:07

AlfredBroda changed the title ~~Enable setting environment variables via mesos.taskInfo~~ Enable setting environment variables via Hooks Nov 30, 2017

janisz suggested changes Nov 30, 2017

View reviewed changes

AlfredBroda force-pushed the feature_set_env branch from aae731b to 42947c0 Compare November 30, 2017 14:38

AlfredBroda force-pushed the master branch from 6705a3a to 4d1a9e4 Compare December 1, 2017 07:52

AlfredBroda force-pushed the feature_set_env branch from 816e858 to bd84cbe Compare December 1, 2017 13:28

janisz previously requested changes Dec 1, 2017

View reviewed changes

medzin reviewed Dec 1, 2017

View reviewed changes

medzin suggested changes Dec 1, 2017

View reviewed changes

AlfredBroda force-pushed the feature_set_env branch from bd84cbe to 7b2fad2 Compare December 1, 2017 14:20

medzin suggested changes Dec 1, 2017

View reviewed changes

AlfredBroda force-pushed the feature_set_env branch from cce9fd0 to 910c347 Compare December 1, 2017 14:45

AlfredBroda force-pushed the feature_set_env branch from 910c347 to de3e296 Compare December 4, 2017 09:38

medzin approved these changes Dec 4, 2017

View reviewed changes

Enable setting environment variables via Hooks

2474a3b

AlfredBroda force-pushed the feature_set_env branch from de3e296 to 2474a3b Compare December 4, 2017 12:28

AlfredBroda merged commit 7cd3c84 into master Dec 4, 2017

janisz reviewed Dec 4, 2017

View reviewed changes

AlfredBroda deleted the feature_set_env branch January 31, 2018 09:02


		returnedValue := taskInfo.FindEnvValue(key)

		require.Equal(t, returnedValue, expectedValue)

Enable setting environment variables via Hooks #27

Enable setting environment variables via Hooks #27

Conversation

AlfredBroda commented Nov 30, 2017

coveralls commented Nov 30, 2017

coveralls commented Nov 30, 2017

medzin left a comment

Choose a reason for hiding this comment

coveralls commented Nov 30, 2017

janisz left a comment

Choose a reason for hiding this comment

coveralls commented Nov 30, 2017

medzin commented Dec 1, 2017 • edited Loading

coveralls commented Dec 1, 2017

AlfredBroda commented Dec 1, 2017

coveralls commented Dec 1, 2017

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

medzin Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

medzin Dec 1, 2017 • edited Loading

Choose a reason for hiding this comment

medzin left a comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2017

medzin commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Dec 1, 2017

coveralls commented Dec 1, 2017

coveralls commented Dec 4, 2017

coveralls commented Dec 4, 2017

janisz 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

medzin commented Dec 1, 2017 •

edited

Loading

medzin Dec 1, 2017 •

edited

Loading

medzin Dec 1, 2017 •

edited

Loading