Support for the --kill-nailguns option was inadvertently removed, this puts it back.

Review Request #1352 — Created Nov. 18, 2014 and submitted

zundel
pants
zundel/nailgun-timeouts-on-ci
803
cb9122a...
pants-reviews
benjyw, ity, patricklaw

The kill-nailguns flag was being read through old_options.cleanup_nailguns

Also updated the nailgun timeout logic to try and pinpoint transient errors in CI if it
happens again.

This reverts the change at https://rbcommons.com/s/twitter/r/1345/ which is redundant with this one (and not as effective.)

CI Passed.

  • 0
  • 0
  • 1
  • 0
  • 1
Description From Last Updated
PA
  1. lgtm

  2. I would really love to get this into config. We find ourselves pushing these limits somewhat regularly when CI machines get a spike of jobs.

    1. I'd like this too. The nailgun_executor class has no notion of context or deriving from some class with access to the 'config' or 'options' objects. I didn't see a 'global' way to get at the global options.

  3. Might as well flip this over to use .format for consistency with below.

    1. kinda went overboard on this one.

  4. 
      
ZU
ZU
  1. 
      
  2. Note: The overall timeout is the same (50 seconds), but I've lengthened the initial connect timeout to 10 seconds from 5 seconds.

    Patrick and I discussed making this configurable a bit. We could fetch the config with Config.from_cache(), but what we really want is a way to reference the new options system.

  3. 
      
ZU
IT
  1. Ship It!

  2. 
      
PA
  1. Ship It!

  2. 
      
ZU
Review request changed

Status: Closed (submitted)

Change Summary:

commit 126360f
Loading...