Darrenge safeslack

[darrenge]

This is for issue #5. Used Irina's sed code with work around for a known sed bug.

[msprotz]

Right, so let me expand on my earlier comments. The pull request fixes the issue and this is great! However, I think we can improve your code to make it even more useful. Here's some things that concern me:

• if someone renames Author into Committer, or swaps the order of the Message and Author fields, then your series of sed will break;
• if someone else, in some other part of the script, wishes to reuse the quoting function (because, say, they want to attach the last line from stderr into the notification message), then they will have to duplicate the logic
• I can read the series of replacements but there's some inherent complexity, with the sed bug you mention.

Here's a suggestion (untested code, of course):

function quote() {
  local msg="$1"
  msg=${msg//&//&}
  msg=${msg//"//"}
  msg=${msg//<//&lt;}
  msg=${msg//>//&gt;}
  echo $msg
}

then, at line 188 of the script, replace:

      *Message:* $slack_log\n\

with:

      *Message:* $(quote slack_log)\n\

This makes the quoting logic re-usable -- as we improve CI, it seems likely that some other parts of the script will need such a piece of functionality; this makes the quoting logic conceptually distinct from the slack-sending function; this makes the matching less fragile by making sure we quote exactly where we need to.

What do you think?

[darrenge]

I understand the reasoning behind moving it to a separate function for reusablility purposes and that makes sense. However, we still can't just send the whole slack package through because it would swap out all the < and > which breaks the commit link in the first part of the slack package and breaks the stderr and stdout+stderr links at the end of the package. That is why we would have to parse out the middle "message" part of the slack package. What we can do ... move it to a separate function and then in post_to_slack still have it parse the message string out and use that separate function to scrub it.

[msprotz]

I'm not following. I'm suggesting to call quote exactly at the right place (see the line number I mention) so that you don't have to reverse-engineer which subset of the string to quote...? Or maybe I'm missing something?

[darrenge]

Ok ... I see what you are talking about now. Will look into implementing it this way.

[msprotz]

That looks good but why are you not quoting "? My intuition would be that this would completely defeat JSON parsing (see https://github.com/project-everest/everest-ci/blob/master/ci#L68) -- if there's a double quote in the commit message, this will generate invalid JSON.

Perhaps the double-quote needs to be quoted (for JSON) as \" instead of "? I haven't checked the Slack documentation.

[darrenge]

We originally wanted to strip for the slack notification to display those properly.

From the Slack docs: 3 Characters you must encode as HTML entities There are three characters you must convert to HTML entities and only three: &, <, and >. Slack will take care of the rest. Replace the ampersand, &, with & Replace the less-than sign, < with < Replace the greater-than sign, > with > That's it. Don't HTML entity-encode the entire message

However, if there is a double quote in the commit line that will break the JSON and create an invalid package. We can't do a " as it will show as " instead of " in the slack notification. Therefore needed to put \" in there instead and that is working.

changes pushed

[msprotz]

That looks great!