add default subscription or sensu server will crash #23

gbolo · 2017-02-22T04:47:42Z

This PR fixes #22

Basically this PR forces the client to send at least one subscription in the form: client:name. Testing and works with both configuration file and environment variables. this default subscription will be appended to subscription slice even if none are specified. This matches the default behaviour of the official ruby sensu client.

Can be tested like:

git clone https://github.com/gbolo/dockerfiles.git
cd dockerfiles/sensu/
docker-compose up -d
docker logs sensu_sensu-client-go-fix_1 -f

Dashboard can be seen: http://127.0.0.1:3000/#/clients

AlexisMontagne

Good work! I gave you some syntax feedbacks

AlexisMontagne · 2017-02-22T14:46:15Z

sensu/config.go

+	} else {
+		// if we get a Name, let's also add the default subscription - client:name
+		// without at least one subscription sensu server will crash
+		cfg.Client().Subscriptions = append(cfg.Client().Subscriptions, "client:"+cfg.Client().Name)


prefer fmt.Sprintf over string concat: fmt.Sprintf("client:%s", cfg.Client().Name)

try to keep your line smaller than 80chars

PS: because you already added this subscription in the Client() method, you don't need to add it again here

AlexisMontagne · 2017-02-22T14:52:28Z

sensu/config.go

 	return &client.Client{
 		Name:          os.Getenv("SENSU_CLIENT_NAME"),
 		Address:       fetchEnv("SENSU_CLIENT_ADDRESS", "SENSU_ADDRESS"),
-		Subscriptions: split(os.Getenv("SENSU_CLIENT_SUBSCRIPTIONS"), ","),
+		Subscriptions: cs,


you don't need to assign all these variables assignations you can easily build all the subscriptions in here:

Subscriptions: append( split(os.Getenv("SENSU_CLIENT_SUBSCRIPTIONS"), ","), fmt.Sprintf("client:%s", cfg.Client().Name), )

gbolo · 2017-02-22T23:47:54Z

hi @AlexisMontagne

Thank you for your review. My apologies for the bad code, as I am completely new to golang and not a developer. I will clean it up when I get home later, I think I understand the flow better now. This a great learning opportunity for me. Thanks for a great go sensu client btw.

gbolo · 2017-02-23T03:41:14Z

hi @AlexisMontagne

I have amended my last commit. I now do the following:

Upon determining that our client section of the configuration has a name and after client() method is called which finally determines the final state of the Subscriptions slice from user input (config file or env vars), I then append the Subscriptions slice and also remove any duplicate slices that may occur due to user error or introduced by the append. This duplicates the behaviour of the ruby client which also seems to remove duplicate subscriptions

AlexisMontagne · 2017-02-23T14:32:31Z

sensu/config.go

+	// If we reached this point, we have a client name.
+	// Let's also add the default subscription - client:name
+	// Without at least one subscription sensu server will crash.
+	cfg.config.Client.Subscriptions = append(cfg.config.Client.Subscriptions,


To write multi line method call, please write it as:

append( arg1, arg2, )

if you need at least one subscriptions you don't need to add the value all the time just ensure len(cfg.config.Client.Subscriptions) == 0

hi @AlexisMontagne,
the requirement for at least one subscription comes from the sensu server crashing, but if we want to duplicate the behaviour of the ruby client we must always add this subscription client:name. I tested a config with other subscriptions and it appears like the ruby client still adds it.

The choice is yours if you would like to copy the behaviour of the ruby client. Please let me know before I amend my commit later. In my opinion we should behave similarly.

AlexisMontagne · 2017-02-23T14:44:25Z

Hey @gbolo,

Could you also add some test to ensure the behavour of your feature? ( https://github.com/upfluence/sensu-client-go/blob/master/sensu/config_test.go ) It would be great!

gbolo · 2017-02-23T14:54:01Z

hi @AlexisMontagne,

I will amend the commit later to fix the styling error you described for append(). I will also add a second commit to address the lack of test. Thanks for you patience

AlexisMontagne · 2017-02-23T14:57:26Z

sensu/config.go

@@ -131,3 +141,17 @@ func (c *Config) Checks() []*check.Check {

 	return []*check.Check{}
 }
+
+// Removes duplicates from string slices
+func RemoveDuplicates(xs *[]string) {


Could you rewrite this function and make it something immutable. removeDuplicates([]string) []string

And it does not need to be public, please use a lowercase for the first letter of the function

AlexisMontagne · 2017-02-23T20:57:35Z

sensu/config.go

+
+// Removes duplicates from string slices
+func removeDuplicates(xs []string) []string {
+	found := make(map[string]bool)


a simpler implementation would be:

var ( temp = map[string]bool{} result = []string ) for _, x := range xs { temp[x] = true } for k, _ := range temp { result = append(result, k) } return result

added your preferred function logic

AlexisMontagne · 2017-02-23T20:59:49Z

sensu/config.go

+	// If we reached this point, we have a client name.
+	// Let's also add the default subscription - client:name
+	// Without at least one subscription sensu server will crash.
+	cfg.config.Client.Subscriptions = append(


do you really need to re assign cfg.config.Client.Subscriptions twice?

you could do both at the same time:

cfg.config.Client.Subscriptions = removeDuplicates( append( cfg.config.Client.Subscriptions, fmt.Sprintf("client:%s", cfg.config.Client.Name), ), )

AlexisMontagne · 2017-02-27T15:42:35Z

@gbolo The code looks good to me, could you add some tests to assert it?

gbolo · 2017-02-27T16:11:49Z

@AlexisMontagne thanks for comment. I was thinking about how to test. Since the additional code only exists in NewConfigFromFile function, the usual test cases which call Client() method won't apply. Can you suggest what the best way to test this is? I am totally new to this and would appreciate some guidance. Thanks

AlexisMontagne · 2017-02-27T16:33:22Z

@gbolo you should write some configuration fixtures (json files) in a directory ( something like testdata/ ) and the idea is to assert the client built out of it.

write some test case with the following format:

for _, tCase := range []struct{
   in string
   out client.Client
}{
  {"testdata/fixtures1.json", client.Client{...}},
} {
  cfg, err := NewConfigFromFile(nil, tCase.in)

  if err ...
 
  if !reflet.DeepEqual(cfg.Client(), tCase.out) ...
}

gbolo · 2017-02-28T04:17:29Z

Hi @AlexisMontagne, I have added a working test case.

AlexisMontagne · 2017-02-28T15:28:21Z

@gbolo could you test multiple cases:

empty subscriptions case
uniq subscriptions
duplicaticates

gbolo · 2017-02-28T16:37:31Z

@AlexisMontagne sure, I will add these additional cases tonight.

…bscriptions

gbolo · 2017-03-01T03:00:56Z

@AlexisMontagne the additional test cases have been added.

AlexisMontagne · 2017-03-01T14:54:54Z

@gbolo One of the best practice to write test in Go, is to write some structured test cases ( input + output ) and test it consistently. cf. https://github.com/golang/go/wiki/TableDrivenTests

could you update your tests according to it.

PS: in my last comment i gave you an example of what the tests could look like.

gbolo · 2017-03-02T03:03:31Z

Hi @AlexisMontagne,

Thanks for the link, I read over it and redesigned the test cases. Good learning experience. Hopefully we are done with this PR so I can begin working on adding the much needed TLS support.

gbolo · 2017-03-06T15:16:01Z

Hi @AlexisMontagne, are any other modifications needed, or can this be merged?

mihaitodor · 2017-03-28T15:04:44Z

@AlexisMontagne Do you think we can merge this? I am adding some code for upfluence/sensu-go#1 and it would be great to get this merged first.

AlexisMontagne · 2017-03-28T15:11:03Z

@mihaitodor I totally forgot about this PR, i'm sorry! I will merge it right away

mihaitodor · 2017-03-28T15:11:59Z

Thanks a lot :)

gbolo mentioned this pull request Feb 22, 2017

this go client seems to crash the sensu server #22

Closed

AlexisMontagne reviewed Feb 22, 2017

View reviewed changes

gbolo force-pushed the master branch 2 times, most recently from 83633e0 to e0aed7d Compare February 23, 2017 03:33

AlexisMontagne reviewed Feb 23, 2017

View reviewed changes

gbolo force-pushed the master branch from e0aed7d to ba57dfa Compare February 23, 2017 19:21

AlexisMontagne reviewed Feb 23, 2017

View reviewed changes

gbolo force-pushed the master branch from ba57dfa to 699f957 Compare February 24, 2017 03:24

add default subscription and remove duplicates

1c9d08b

gbolo force-pushed the master branch from 699f957 to 1c9d08b Compare February 24, 2017 03:45

Adding test case for upfluence#22 default & duplicate subscriptions

d059cdc

AlexisMontagne added reviewed/needs-work and removed reviewed/needs-work labels Feb 28, 2017

Adding more test cases: TestClientNoSubscriptions, TestClientUniqueSu…

8355961

…bscriptions

rewrote test cases for subscription behaviour

91af12f

AlexisMontagne merged commit 3b96af2 into upfluence:master Mar 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add default subscription or sensu server will crash #23

add default subscription or sensu server will crash #23

gbolo commented Feb 22, 2017 •

edited

Loading

AlexisMontagne left a comment

AlexisMontagne Feb 22, 2017

AlexisMontagne Feb 22, 2017

AlexisMontagne Feb 22, 2017

gbolo commented Feb 22, 2017

gbolo commented Feb 23, 2017

AlexisMontagne Feb 23, 2017

AlexisMontagne Feb 23, 2017

gbolo Feb 23, 2017 •

edited

Loading

gbolo Feb 23, 2017

AlexisMontagne commented Feb 23, 2017

gbolo commented Feb 23, 2017

AlexisMontagne Feb 23, 2017

gbolo Feb 23, 2017 •

edited

Loading

AlexisMontagne Feb 23, 2017

gbolo Feb 24, 2017

AlexisMontagne Feb 23, 2017

gbolo Feb 24, 2017

AlexisMontagne commented Feb 27, 2017

gbolo commented Feb 27, 2017

AlexisMontagne commented Feb 27, 2017

gbolo commented Feb 28, 2017

AlexisMontagne commented Feb 28, 2017

gbolo commented Feb 28, 2017

gbolo commented Mar 1, 2017

AlexisMontagne commented Mar 1, 2017

gbolo commented Mar 2, 2017

gbolo commented Mar 6, 2017

mihaitodor commented Mar 28, 2017

AlexisMontagne commented Mar 28, 2017

mihaitodor commented Mar 28, 2017

add default subscription or sensu server will crash #23

add default subscription or sensu server will crash #23

Conversation

gbolo commented Feb 22, 2017 • edited Loading

AlexisMontagne 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

gbolo commented Feb 22, 2017

gbolo commented Feb 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gbolo Feb 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexisMontagne commented Feb 23, 2017

gbolo commented Feb 23, 2017

Choose a reason for hiding this comment

gbolo Feb 23, 2017 • 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

AlexisMontagne commented Feb 27, 2017

gbolo commented Feb 27, 2017

AlexisMontagne commented Feb 27, 2017

gbolo commented Feb 28, 2017

AlexisMontagne commented Feb 28, 2017

gbolo commented Feb 28, 2017

gbolo commented Mar 1, 2017

AlexisMontagne commented Mar 1, 2017

gbolo commented Mar 2, 2017

gbolo commented Mar 6, 2017

mihaitodor commented Mar 28, 2017

AlexisMontagne commented Mar 28, 2017

mihaitodor commented Mar 28, 2017

gbolo commented Feb 22, 2017 •

edited

Loading

gbolo Feb 23, 2017 •

edited

Loading

gbolo Feb 23, 2017 •

edited

Loading