1369 - Integrate ensure_dependencies.py

See https://issues.adblockplus.org/ticket/1369.

[Wladimir Palant, 2014-09-18 10:45:59]

People won't remember to run ensure_dependencies.py manually, please change
Vagrantfile to make sure it does so automatically.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
File dependencies (right):

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
dependencies:2: _root = hg:https://hg.adblockplus.org/
#git:https://github.com/adblockplus/
You should be able to add the corresponding Git revisions now.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
dependencies:4: #_self = buildtools/ensure_dependencies.py
Please remove this setting as it doesn't apply here.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
dependencies:6: # at tag "1.2".
This comment makes no sense outside the example.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
dependencies:17: #buildtools = buildtools hg:tip git:tip
Please remove this, we don't want a dependency from infrastructure to
buildtools.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/ensu...
File ensure_dependencies.py (right):

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/ensu...
ensure_dependencies.py:78: subprocess.check_call(["git", "clone", "--quiet",
"--no-checkout", source, target])
You probably want to wait until https://issues.adblockplus.org/ticket/1366 is
fixed.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/kick.py
File kick.py (right):

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/kick...
kick.py:52: 'sudo /etc/puppet/infrastructure/ensure_dependencies.py
/etc/puppet/infrastructure',
The sudoers file on puppetmaster needs to be updated, otherwise this will
trigger a password prompt.

[mathias, 2014-09-18 12:59:57]

Updates since last patch set:
1369 - Invoke ensure_dependencies.py before setting up Vagrant
1369 - Include update_dependencies.py in sudoers file for puppetmaster
1369 - Use explicit hg:* and git:* revision IDs
1369 - Merge modifications from issue 1366

[mathias, 2014-09-18 13:01:24]

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
File dependencies (right):

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
dependencies:2: _root = hg:https://hg.adblockplus.org/
#git:https://github.com/adblockplus/
On 2014/09/18 10:45:59, Wladimir Palant wrote:
> You should be able to add the corresponding Git revisions now.

Done.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
dependencies:4: #_self = buildtools/ensure_dependencies.py
On 2014/09/18 10:45:59, Wladimir Palant wrote:
> Please remove this setting as it doesn't apply here.

Done.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
dependencies:6: # at tag "1.2".
On 2014/09/18 10:45:59, Wladimir Palant wrote:
> This comment makes no sense outside the example.

Done.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/depe...
dependencies:17: #buildtools = buildtools hg:tip git:tip
On 2014/09/18 10:45:59, Wladimir Palant wrote:
> Please remove this, we don't want a dependency from infrastructure to
> buildtools.

Done.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/ensu...
File ensure_dependencies.py (right):

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/ensu...
ensure_dependencies.py:78: subprocess.check_call(["git", "clone", "--quiet",
"--no-checkout", source, target])
On 2014/09/18 10:45:59, Wladimir Palant wrote:
> You probably want to wait until https://issues.adblockplus.org/ticket/1366 is
> fixed.

Done.

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/kick.py
File kick.py (right):

http://codereview.adblockplus.org/4768985027444736/diff/5629499534213120/kick...
kick.py:52: 'sudo /etc/puppet/infrastructure/ensure_dependencies.py
/etc/puppet/infrastructure',
On 2014/09/18 10:45:59, Wladimir Palant wrote:
> The sudoers file on puppetmaster needs to be updated, otherwise this will
> trigger a password prompt.

Done.

[Wladimir Palant, 2014-09-18 13:24:46]

http://codereview.adblockplus.org/4768985027444736/diff/5685265389584384/Vagr...
File Vagrantfile (right):

http://codereview.adblockplus.org/4768985027444736/diff/5685265389584384/Vagr...
Vagrantfile:2: system("python ensure_dependencies.py")
This assumes that Vagrantfile is located in the current work directory. However,
vagrant will also find Vagrantfile in the parent of the current work directory -
plus there is a VAGRANT_CWD environment variable that will change that even
further. Please make sure to resolve the path to ensure_dependencies.py relative
to __FILE__.

Also, please make sure to pass in multiple parameters rather than a shell
command - no point getting the shell involved here (potential security hazard).

Finally, a failure in ensure_dependencies.py should not be ignored - we don't
want to produce a broken VM. So it should be something like:

  raise "ensure_dependencies.py failed" if not system("python", deps)

[mathias, 2014-09-18 13:42:40]

Updates since last patch set:
1369 - Fix wrong git:* commit id for infrastructure-puppet-concat

[mathias, 2014-09-18 14:10:56]

Updates since last patch set:
1369 - Fix wrong git:* commit id for infrastructure-puppet-concat
1369 - Improve Vagrant(file) dependency hook

[mathias, 2014-09-18 14:11:20]

http://codereview.adblockplus.org/4768985027444736/diff/5685265389584384/Vagr...
File Vagrantfile (right):

http://codereview.adblockplus.org/4768985027444736/diff/5685265389584384/Vagr...
Vagrantfile:2: system("python ensure_dependencies.py")
On 2014/09/18 13:24:46, Wladimir Palant wrote:
> This assumes that Vagrantfile is located in the current work directory.
However,
> vagrant will also find Vagrantfile in the parent of the current work directory
-
> plus there is a VAGRANT_CWD environment variable that will change that even
> further. Please make sure to resolve the path to ensure_dependencies.py
relative
> to __FILE__.
> 
> Also, please make sure to pass in multiple parameters rather than a shell
> command - no point getting the shell involved here (potential security
hazard).
> 
> Finally, a failure in ensure_dependencies.py should not be ignored - we don't
> want to produce a broken VM. So it should be something like:
> 
>   raise "ensure_dependencies.py failed" if not system("python", deps)

Done.

[Wladimir Palant, 2014-09-18 15:00:05]

LGTM