Ask for confirmation before committing.

Ask for confirmation before committing.

BUG=

Committed: https://chromium.googlesource.com/infra/infra/+/b14b65cdff6cc607b209865af52e830d7a55b29d

[ghost stip (do not use), 2015-07-18 01:04:38]

stip@chromium.org changed reviewers:
+ hinoka@chromium.org

[ghost stip (do not use), 2015-07-18 01:04:38]

ptal!

[hinoka, 2015-07-18 01:15:24]

I'm kind of meh about this.  Typing the command into the cli is already a big
indication that the user intended on restarting that master.  I feel like this
just introduces unnecessary friction for an unlikely scenario.  (Though I could
be wrong about the unlikeliness).  At best this just trains everyone to always
run the command with "-f" thus defeating the purpose.

What about a little printout after the run that says "If you didn't mean to do
this, go to <this Rietveld page> and click revert patchset"?

[estaab, 2015-08-12 01:01:32]

On 2015/07/18 01:15:24, hinoka wrote:
> I'm kind of meh about this.  Typing the command into the cli is already a big
> indication that the user intended on restarting that master.  I feel like this
> just introduces unnecessary friction for an unlikely scenario.  (Though I
could
> be wrong about the unlikeliness).  At best this just trains everyone to always
> run the command with "-f" thus defeating the purpose.
> 
> What about a little printout after the run that says "If you didn't mean to do
> this, go to <this Rietveld page> and click revert patchset"?

I actually like this CL, I think it's pretty reasonable to ask the user to spot
check the list before committing since it can be a dangerous command. I'm not
sure why prompting would encourage -f since it's fewer keystrokes to press enter
than to add the -f to the previous command, and people shouldn't be running this
so frequently to need to save the couple seconds of spot checking. Put another
way, I'm afraid of this command without the confirmation and would never use -f.
:)

[Ryan Tseng, 2015-09-08 22:10:38]

hinoka@google.com changed reviewers:
+ hinoka@google.com

[Ryan Tseng, 2015-09-08 22:10:39]

ok after sitting behind a couple people while they used this for the first time,
I reverse my position.  The lack of confirmation is indeed surprising.

lgtm + suggestions.

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
File infra/tools/restart/restart.py (right):

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
infra/tools/restart/restart.py:37: '--force', action='store_true',
+ short arg "-f"

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
infra/tools/restart/restart.py:66: print 'Restarting the following masters at
%s' % timestring
Suggestion:
Restarting the following masters in 5 minutes (<timestring>)

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
infra/tools/restart/restart.py:69: print
+print "This will upload a CL for master_manager.git, TBR <person>, and commit
the CL through the CQ"

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
infra/tools/restart/restart.py:74: if input_string != '' and not
distutils.util.strtobool(input_string):
https://big.corp.google.com/~jmcmaster/testing/2015/07/episode-387-improving-...

if input_string == '' or distutil......:
  positive stuff
  return

print 'Aborting'

[ghost stip (do not use), 2015-09-09 00:08:54]

ptal

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
File infra/tools/restart/restart.py (right):

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
infra/tools/restart/restart.py:37: '--force', action='store_true',
On 2015/09/08 22:10:39, Ryan Tseng wrote:
> + short arg "-f"

Done.

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
infra/tools/restart/restart.py:66: print 'Restarting the following masters at
%s' % timestring
On 2015/09/08 22:10:39, Ryan Tseng wrote:
> Suggestion:
> Restarting the following masters in 5 minutes (<timestring>)

Done.

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
infra/tools/restart/restart.py:69: print
On 2015/09/08 22:10:39, Ryan Tseng wrote:
> +print "This will upload a CL for master_manager.git, TBR <person>, and commit
> the CL through the CQ"

Done.

https://chromiumcodereview.appspot.com/1233243007/diff/1/infra/tools/restart/...
infra/tools/restart/restart.py:74: if input_string != '' and not
distutils.util.strtobool(input_string):
On 2015/09/08 22:10:39, Ryan Tseng wrote:
>
https://big.corp.google.com/~jmcmaster/testing/2015/07/episode-387-improving-...
> 
> if input_string == '' or distutil......:
>   positive stuff
>   return
> 
> print 'Aborting'

problem is, this has to return to stop control flow (otherwise you'd have to
nest the if not force in there). I tried to split it into two, but the logic
just doesn't work out

[ghost stip (do not use), 2015-09-09 00:09:30]

The CQ bit was checked by stip@chromium.org

[ghost stip (do not use), 2015-09-09 00:09:30]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com
Link to the patchset: https://chromiumcodereview.appspot.com/1233243007/#ps20001
(title: "Address hinoka's comments.")

[commit-bot: I haz the power, 2015-09-09 00:09:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233243007/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1233243007/20001

[commit-bot: I haz the power, 2015-09-09 00:10:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-09-09 00:10:35]

Try jobs failed on following builders:
  Infra Presubmit on tryserver.infra (JOB_FAILED,
https://build.chromium.org/p/tryserver.infra/builders/Infra%20Presubmit/build...)

[ghost stip (do not use), 2015-09-09 00:19:48]

The CQ bit was checked by stip@chromium.org

[ghost stip (do not use), 2015-09-09 00:19:49]

The patchset sent to the CQ was uploaded after l-g-t-m from hinoka@google.com
Link to the patchset: https://codereview.chromium.org/1233243007/#ps40001
(title: "Rebase onto latest master.")

[commit-bot: I haz the power, 2015-09-09 00:19:53]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1233243007/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1233243007/40001

[commit-bot: I haz the power, 2015-09-09 00:23:30]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/infra/infra/+/b14b65cdff6cc607b209865af52e8...