prevented grimServerID from being over 15 chars #62

harisgodil-MM · 2015-06-10T19:52:01Z

Truncates the grimServerID and logs that it was truncated and how to fix it

kklipsch · 2015-06-10T20:26:55Z

notify.go

+		grimServerID := config.grimServerID
+		if len(grimServerID) > 15 {
+			grimServerID = fmt.Sprintf("%s", grimServerID[:15])
+			logger.Printf("grimServerID shouldn't be over 15 characters and has been truncated\nPlease update your config.json file to have a shorter grimServerID\n")


grimServerID is the name of the in memory field. In the config it is actually GrimServerID. Also, note it is possible to get the GrimServerID set by the "defaults" which is hte grim queue name, so they might not even have a GrimServerID. Don't know what to do about that, maybe add to the error message?

Also note, you haven't truncated the grimServerID globally, only for the first message. So the next time they do a notify (like 3 lines later) it will trigger again.

So, you should move this code to the config file validation/load code. That will cause it to trigger on startup, not on the first notify message.

kklipsch · 2015-06-11T16:34:35Z

notify.go

 			return err
 		}

-		err = sendMessageToRoom(config.hipChatToken, config.hipChatRoom, config.grimServerID, message, color)
+		grimServerID := config.grimServerID


grimServerID is the name of the in memory field. In the config it is actually GrimServerID. Also, note it is possible to get the GrimServerID set by the "defaults" which is hte grim queue name, so they might not even have a GrimServerID. Don't know what to do about that, maybe add to the error message?

Also note, you haven't truncated the grimServerID globally, only for the first message. So the next time they do a notify (like 3 lines later) it will trigger again.

So, you should move this code to the config file validation/load code. That will cause it to trigger on startup, not on the first notify message.

kklipsch · 2015-06-12T13:40:35Z

config.go

@@ -19,6 +19,7 @@ var (
 	defaultResultRoot         = "/var/log/grim"
 	defaultWorkspaceRoot      = "/var/tmp/grim"
 	configFileName            = "config.json"
+	truncatedGrimServerID     = ""


You never use this (and you shouldn't as it is a global variable).

kklipsch · 2015-06-12T13:50:21Z

What you've done is:

created a global variable.
Set it on startup.

But it isn't actually being used anywhere. So the hipchat notification will still see and use the longer than 15 character name.

What you should do:

Validate/truncate in the loading of the config file https://github.com/MediaMath/grim/blob/master/config.go#L150
Log that you've done this truncation. This will require adding either a logger injected into that function (which may be painful) or a special error that you return with that function and look at to determine if it is an "ok" error (which may be dangerous).

One option might be to add a field to the effective config struct that is "hipchatGrimServerId" and do the truncation into that struct. Then for logging on startup right after loading you can check to see if grimServerID != hipchatGrimServerId and log in that case.

Then in the notify function you should use the truncated value.

harisgodil-MM · 2015-06-12T15:18:47Z

I made a slight change to the implementation from what @kklipsch said I should do
Rather than put the truncated value into hipchatGrimServerID and have the grimServerID be unmodified, I put the truncated value into grimServerID and the original to origServerID. This way grimServerID would be used the same way it was before

mmacdermaid · 2015-06-12T15:53:08Z

grim_test.go

+	var buf bytes.Buffer
+	logger := log.New(&buf, "", log.Lshortfile)
+
+	tempDir, _ := ioutil.TempDir("", "TestTruncatedGrimServerID")


kklipsch · 2015-06-15T14:05:36Z

config.go

 	return effectiveConfig{
 		grimQueueName:   firstNonEmptyStringPtr(global.GrimQueueName, &defaultGrimQueueName),
 		resultRoot:      firstNonEmptyStringPtr(global.ResultRoot, &defaultResultRoot),
 		workspaceRoot:   firstNonEmptyStringPtr(global.WorkspaceRoot, &defaultWorkspaceRoot),
-		grimServerID:    firstNonEmptyStringPtr(global.GrimServerID, global.GrimQueueName, &defaultGrimQueueName),
+		grimServerID:    truncatedServerID,


My own personal preference is to leave the config as it was created by the user. And to store the truncated in a new value. That way there is no confusion about what the user entered.

That said it is a nitpick and you don't have to change it.

harisgodil-MM · 2015-06-15T15:26:09Z

Logger now knows which parameter in the config.json file was too large (GrimServerID and GrimQueueName) and uses it in the logged message
With that, the message became too complex to be stored as a global variable, so i made a method to create the message

kklipsch · 2015-06-15T15:37:00Z

grim.go

@@ -218,3 +222,9 @@ func (i *Instance) checkGrimQueue() error {

 	return nil
 }
+
+func BuildTruncatedMessage(truncateID string) string {


This function doesnt need to be exported. If it is exported it needs to be commented. golint would tell you this so I suspect you are not running golint? (make check will do this for you)

bhand-mm · 2015-06-15T16:22:30Z

config.go

+			truncateID = "GrimQueueName"
+		}
+	}
+


Personally I think I'd like to see this pulled out into a function called performCorrections (or something to that effect) that takes an effectiveConfig and returns (effectiveConfig, []error). That way the caller has the before/after and the messages that it can log or throw away.

Don't worry about it unless @kklipsch agrees though. This implementation is solid as is.

Whenever we get an effectiveConfig, we are reading it from the file. If we go with this approach, we would have to call performCorrections every time we use an effectiveConfig.

prevented grimServerID from being over 15 chars

harisgodil-MM mentioned this pull request Jun 10, 2015

grimServerId should not be > 15 characters #42

Closed

kklipsch reviewed Jun 10, 2015
View reviewed changes

harisgodil-MM force-pushed the master branch from e8278a7 to 9f98c10 Compare June 10, 2015 21:16

harisgodil-MM mentioned this pull request Jun 11, 2015

logdir will now show full path to results dir #64

Merged

kklipsch reviewed Jun 11, 2015
View reviewed changes

harisgodil-MM force-pushed the master branch 2 times, most recently from 7a99f98 to f3aa807 Compare June 11, 2015 22:02

kklipsch reviewed Jun 12, 2015
View reviewed changes

harisgodil-MM force-pushed the master branch from f3aa807 to 1ee0b9f Compare June 12, 2015 15:14

mmacdermaid reviewed Jun 12, 2015
View reviewed changes

harisgodil-MM force-pushed the master branch from 7b61cf3 to bc9e47e Compare June 12, 2015 20:22

kklipsch reviewed Jun 15, 2015
View reviewed changes

harisgodil-MM force-pushed the master branch from 7f60946 to e1abdf1 Compare June 15, 2015 15:29

kklipsch reviewed Jun 15, 2015
View reviewed changes

harisgodil-MM force-pushed the master branch 2 times, most recently from 5ef9767 to cb90f40 Compare June 15, 2015 15:54

bhand-mm reviewed Jun 15, 2015
View reviewed changes

harisgodil-MM force-pushed the master branch from 4a07df2 to da5717c Compare June 16, 2015 19:53

fixes MediaMath#42

d4c4749

harisgodil-MM force-pushed the master branch from da5717c to d4c4749 Compare June 16, 2015 20:00

kklipsch added a commit that referenced this pull request Jun 16, 2015

Merge pull request #62 from harisgodil-MM/master

835c154

prevented grimServerID from being over 15 chars

kklipsch merged commit 835c154 into MediaMath:master Jun 16, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevented grimServerID from being over 15 chars #62

prevented grimServerID from being over 15 chars #62

harisgodil-MM commented Jun 10, 2015

kklipsch Jun 10, 2015

kklipsch Jun 11, 2015

kklipsch Jun 12, 2015

kklipsch commented Jun 12, 2015

harisgodil-MM commented Jun 12, 2015

mmacdermaid Jun 12, 2015

kklipsch Jun 15, 2015

harisgodil-MM commented Jun 15, 2015

kklipsch Jun 15, 2015

bhand-mm Jun 15, 2015

harisgodil-MM Jun 15, 2015

prevented grimServerID from being over 15 chars #62

prevented grimServerID from being over 15 chars #62

Conversation

harisgodil-MM commented Jun 10, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kklipsch commented Jun 12, 2015

harisgodil-MM commented Jun 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

harisgodil-MM commented Jun 15, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment