Add DTS as part of deploy_to_test. #13930
Conversation
bin/cron/deploy_to_test
Outdated
| if pr_number.nil? | ||
| raise Exception.new('GitHub.create_and_merge_pull_request failed.') | ||
| end | ||
| sleep 15 |
There was a problem hiding this comment.
It is totally unclear to me whether this sleep is required. It just seems useful so as to not have a race condition with the GitHub API. Thoughts?
|
👀 Looking... |
There was a problem hiding this comment.
More generally, I'm not sure this is the right approach to that error message, because it's not how our manual DTT process normally works - it shouldn't be a problem for test to be ahead by a few merge commits, as long as there are no conflicts.
I've seen this error in the GitHub UI every once in a while. It seems like a race condition where the base branch changed between the mergeability check and the merge button click, and the solution is usually to run another mergeability check (reloading the page?), not necessarily backmerging. I wonder if our cronjob should sleep 10 and try again in this situation instead of attempting to backmerge.
All that said, I have no idea why this would be an issue for our overnight robo-DTT - something smells fishy here, like there's a timing problem or an incorrect API response. And it makes me wonder if we'll soon be better off moving away from pull requests for deploy merges and merging directly instead (with appropriate affordances for exploring deploy history, of course).
bin/cron/deploy_to_test
Outdated
| @@ -31,6 +31,31 @@ def main | |||
|
|
|||
| DevelopersTopic.set_dtt TOPIC_DTT_IN_PROGRESS | |||
|
|
|||
| unless GitHub.compare(base: 'staging', head: 'test').empty? | |||
There was a problem hiding this comment.
So, I wrote a helper for this yesterday and backed it out because I didn't find a compelling use case - but you've got one here. The compare API returns ahead/behind information which feels like a cleaner way to ask this question.
"status": "behind",
"ahead_by": 1,
"behind_by": 2,
"total_commits": 1,
So maybe GitHub.ahead?(base: 'staging', head: 'test')
bin/cron/deploy_to_test
Outdated
| ) | ||
| pr_number = GitHub.create_and_merge_pull_request( | ||
| base: 'staging', | ||
| head: 'head', |
There was a problem hiding this comment.
I think this should be head: 'test',.
ashercodeorg
force-pushed
the
fixDeployToTest
branch
2 times, most recently
from
e490961
to
e5ad542
Compare
|
PTAL. Note that I intend to update other |
lib/cdo/github.rb
Outdated
| base_sha = sha(base) | ||
| compare_sha = sha(compare) | ||
|
|
||
| response = Octokit.compare(REPO, base_sha, compare_sha) |
There was a problem hiding this comment.
Why are we getting the hashes at all here? Compare should accept branch names. From the docs:
GET /repos/:owner/:repo/compare/:base...:headBoth
:baseand:headmust be branch names in:repo.
There was a problem hiding this comment.
Good question. Changing...
|
New helpers look good, but you haven't addressed my thoughts on whether a backmerge is the right approach at all here. |
|
As suggested, I changed the DTS (Test > Staging) to a sleep between the create and merge. PTAL. |
lib/cdo/github.rb
Outdated
| def self.ahead?(base:, compare:) | ||
| response = Octokit.compare(REPO, base, compare) | ||
| response.ahead_by > 0 | ||
| end |
There was a problem hiding this comment.
This method is now unused. Let me know if you would rather it remain or be removed.
There was a problem hiding this comment.
Hmm. My default instinct is to remove dead code, esp. since we have no test over this. Should be trivial to reconstruct from the behind? method below, too. Let's cut it.
ashercodeorg
force-pushed
the
fixDeployToTest
branch
from
447c6ba
to
2054945
Compare
This attempts to fix an exception (example below) being seen by
deploy_to_testwheretestcontains several merge commits fromstagingthat have not yet been merged back tostaging.