Improve and automate bin/dotd. #13722
Conversation
|
This PR is a large number of small commits. Feel welcome to review the whole set of changes at once, feel welcome (encouraged) to review commit by commit (mild effort was taken to keep them self-contained and small). |
| require_relative '../lib/cdo/github' | ||
| include CdoCli | ||
|
|
||
| LOG_FILE = "#{Dir.tmpdir}/dotd.log" | ||
| LOG_FILE = "#{Dir.tmpdir}/dotd.log".freeze | ||
| # TODO(asher): This is not a constant, so should not be NAMED as one. |
There was a problem hiding this comment.
This TODO seems worth addressing within this PR.
bin/dotd
Outdated
| name | ||
|
|
||
| welcome = " Hi #{@dotd_name}! Please follow along carefully, as options "\ | ||
| "and codepaths are changing often." |
There was a problem hiding this comment.
It drives me crazy when dial-menu systems always say "Please listen carefully, as the menu options have changed" because they usually haven't. I understand the precaution here, but I'm not sure it's worth it - maybe just shorten to "Please follow along carefully."
There was a problem hiding this comment.
Fair enough. Removed.
bin/dotd
Outdated
| "and codepaths are changing often." | ||
| puts '' | ||
| puts welcome | ||
| puts '' |
There was a problem hiding this comment.
Why not heredoc for this block of text?
bin/dotd
Outdated
| # @param log_action [Boolean] Whether to log the action to LOGGER. Default is | ||
| # true. | ||
| # @block The block to run if the user responsds affirmatively. | ||
| def should_i(question, log_action: true) |
There was a problem hiding this comment.
Right now there's exactly one case where we don't want to log the action (resetting the DTT topic to true). But even that might be useful to see in a log. Maybe simpler and safer to just log every action, even those that seem trivial?
There was a problem hiding this comment.
Fair enough. Done.
bin/dotd
Outdated
| exit(-1) | ||
| else | ||
| puts "Sorry, I didn't understand that.\n\n" | ||
| end | ||
| end | ||
| end | ||
|
|
||
| # Prompts the user to press enter, pausing until any key is pressed. | ||
| def press_enter_to_continue |
There was a problem hiding this comment.
Is this comment true? I believe you have to press enter for gets to return, although you could certainly enter other text which would get ignored. Otherwise how does ask_for_name work?
bin/dotd
Outdated
| else | ||
| output_text = red "CURRENT DTT SUBTOPIC: #{DevelopersTopic.dtt_subtopic}" | ||
| end | ||
| puts output_text |
There was a problem hiding this comment.
A few recommendations:
- I would flip this around (see example below)
- "Subtopic" isn't a super well-known term on the team, maybe use something a little simpler.
- I'm not sure the screaming caps are needed, esp. with the red variant when DTT is no.
Proposal:
def print_dtt_subtopic
msg = "The #developers room says DTT: #{DevelopersTopic.dtt_subtopic}"
if DevelopersTopic.dtt?
puts dim msg
else
puts red msg
end
endThere was a problem hiding this comment.
Done (with s/msg/message).
| should_i "DTT" do | ||
| view_commits base: 'test', head: 'staging' | ||
| should_i "merge from staging to test" do | ||
| DevelopersTopic.set_dtt_subtopic "no (@#{@dotd_name} in progress)" |
There was a problem hiding this comment.
For a future PR, probably: Reading this I realize subtopic is mostly redundant since we'll always invoke this on the DevelopersTopic module: DevelopersTopic.set_dtt "no (...)" reads nicely.
There was a problem hiding this comment.
Agreed, will follow up. And I'm regretting GitHub.html_url, I'll also change that to GitHub.url.
bin/dotd
Outdated
| wait_for "DTT (#{url}) to complete, re-run / investigate failures, and "\ | ||
| "update infra#test topic to commit #{new_sha}" | ||
| # TODO(asher): Prompt for whether the DTT was green, update | ||
| # Slack#infra-test topic appropriately. |
There was a problem hiding this comment.
Later, I'd prefer to launch and iterate.
bin/dotd
Outdated
| base: 'staging', head: 'levelbuilder', title: 'DTS (Levelbuilder > Staging)' | ||
| ) | ||
| new_sha = GitHub.sha 'staging' | ||
| new_sha_abbr = new_sha[0, 4] + '...' + new_sha[-4, 4] |
There was a problem hiding this comment.
The git/GitHub convention for abbreviating commit SHAs is new_sha[0, 7] as it's almost always unique - I think that'd be sufficient here.
| # @param head [String] The head branch of the comparison. | ||
| # @param title [String] The title of the candidate pull request. | ||
| # @raise [RuntimeError] If the environment is not development. | ||
| def self.open_pull_request_in_browser(base:, head:, title:) |
There was a problem hiding this comment.
Nit: This name isn't exactly true, since we're opening the compare page to help create a pull request.
There was a problem hiding this comment.
Eh, it has the caveat "in browser". For now, I'm going to punt.
|
Though my changes were completely in line with your suggestions (with the exception of ignoring the naming of |
|
Looks great! |
No description provided.