automatically mark successful DTTs green

[davidsbailey]

The technique for doing this is lifted from

code-dot-org/bin/dotd

Lines 82 to 84 in 701887a

	def mark_dtt_green
	DevelopersTopic.set_dtt 'yes'
	InfraTestTopic.set_green_commit GitHub.sha('test')

. I couldn't quite convince myself that a two-line method was worth extracting.

I tested this by running rake test:mark_dtt_green from the test machine, and by running some other rake commands to confirm my understanding that this step will not be executed if a previous rake step fails.

[islemaster]

I think this might be a mistake, here and in our DOTD script as well. This helper actually uses Octokit to ask Github for the sha of the test branch at the end of the build, but if somebody pushed to the test branch while the build was running I think we could end up with a different sha than the one we actually ran tests against. We may want to ask for the sha of the local test branch instead, or capture this at the start of the build.

[davidsbailey]

Great catch Brad! This is a mistake here. I've fixed it by looking at the HEAD commit on the test machine. I believe this is sufficient because if the test machine bits have changed mid-DTT, then we don't even know if we tested the old or new bits, and marking the original HEAD commit green wouldn't be any more correct.

As for the bin/dotd script, the change I made here wouldn't be very helpful to make there, because by the time the dotd has the opportunity to mark the DTT green, any changes to the test branch would likely have already been pulled down to the test machine. In that case remembering the HEAD commit from when the DTT was initiated could still save our butts in some cases, and is probably a good idea for us to implement. I'm going to say that's out of scope for this PR, though.

[jeremydstone]

Looks like this is the implementation of marking a DTT green but not the triggering logic. Some thoughts on the broader topic that I'll include here: whenever the triggering logic comes around, let's please have a way to make triple sure we never have a process accidentally mark a DTT green. The nightmare is we DTP a build only to discover that tests didn't actually pass. You can imagine bugs that could occur such as finding successful test results, except they are from a previous run. So let's please have the triggering logic cross-check everything it possibly can, be hard to break in the future, and make a mistake in the false-negative direction if it's going to make a mistake at all. It might not be a bad idea to run for a while with the process posting what it would conclude about a DTT without actually marking it green, and have a human verify until we build confidence.

[davidsbailey]

Hi @jeremydstone , apologies for forgetting this recommendation you made earlier. As a temporary measure, I could change this to only log "Marking commit abcd0123 green (temporarily disabled)" to Slack without updating any room topics, and the dotd can keep an eye out for any false positives for a couple weeks worth of deploys. I think this will be visible enough and easy to audit after the fact if this logging happens in the #deploy-status room.

As for triggering logic: it is implemented here, because a rake step will only execute after the previous steps completes successfully. We already rely on this mechanism extensively in our deploy process. For example, this is what keeps us from DTPing if rake db:migrate fails on production-daemon. It's also what keeps a DTT from appearing to be green if a unit test fails.