New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support new vagrant and ansible versions #932

Closed
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@conorsch
Contributor

conorsch commented Mar 23, 2015

This PR makes a one-line change to the Vagrantfile that disables dynamic SSH key substitution in recent (1.7.0+) versions of vagrant. This problem was discussed in #802, and we've been recommending that developers use vagrant 1.6.5 since then. Downgrading isn't necessary, since we can toggle the key substitution by setting:

config.ssh.insert_key = false

All VirtualBox machines managed by the Vagrantfile are affected by this change. If the keys have been substituted already, destroying and recreating the machines is the easiest way to get back in sync. To make sure that setting this option doesn't break provisioning with older versions, I tested a variety of versions of both Ansible and Vagrant. The test consisted of destroying all VMs, setting the desired ansible and vagrant versions, then creating and provisioning all machines.

I was able to create and fully provision all machines with these version combinations:

  • vagrant 1.5.4 & ansible 1.8.4
  • vagrant 1.6.5 & ansible 1.8.4
  • vagrant 1.7.2 & ansible 1.8.4
  • vagrant 1.5.4 & ansible 1.7.2
  • vagrant 1.6.5 & ansible 1.7.2
  • vagrant 1.7.2 & ansible 1.7.2

This version combination failed (due to an apache2_module call):

  • vagrant 1.5.4 & ansible 1.5.4

Based on those results, I'm comfortable recommending the newest versions of both vagrant and ansible, and have updated the development docs to say so.

Caveats

  • We may want to caution folks more strongly against using ansible 1.5.4, since it fails on our plays (apache2_module was added in 1.6) and it's currently the default in the repos for Ubuntu 14.04.
  • Based on my read of the Vagrant docs, setting config.ssh.insert_key = false should not break SSH key insertion via override.ssh.private_key_path, as in the case of the Digital Ocean box, but keep an eye out for regressions on that box just in case.

This change resolves #802 and resolves #906.

@garrettr

This comment has been minimized.

Contributor

garrettr commented Mar 23, 2015

Gotta say this is a great first pull request! And perfect timing too, for reasons that should become clear later this week :)

We may want to caution folks more strongly against using ansible 1.5.4, since it fails on our plays (apache2_module was added in 1.6) and it's currently the default in the repos for Ubuntu 14.04.

Yeah, I agree it would be good to add that. I'll add a commit with a warning about that before merging this.

@ageis

This comment has been minimized.

ageis commented on d62968c Mar 23, 2015

@garrettr let's add a -U argument to sudo pip install ansible so that they will get the latest version if it's already installed. apparently just running install doesn't update for me.

@garrettr

This comment has been minimized.

Contributor

garrettr commented Mar 23, 2015

Added a commit with the warning about Ansible 1.5.4 being default package in Ubuntu 14.04, and ended up cleaning up and clarifying a lot of develop.md while I was at it. I also upgraded Vagrant on my Mac from 1.6.5. to 1.7.2, and can confirm that vagrant destroy -f /staging$/ && vagrant up /staging$/ works with both (Vagrant 1.7.2, Ansible 1.7.2) and then upgraded Ansible to the latest and confirmed that i also works with (Vagrant 1.7.2, Ansible 1.8.4). Nice!

@garrettr

This comment has been minimized.

Contributor

garrettr commented Mar 23, 2015

@ageis K, will do

garrettr added a commit that referenced this pull request Mar 23, 2015

garrettr added a commit that referenced this pull request Mar 23, 2015

@garrettr

This comment has been minimized.

Contributor

garrettr commented Mar 23, 2015

@conorsch FYI, master is only for releases. You always want to merge into develop. We used to have develop be the default branch so this was what happened automatically, but a lot of people have requested that we make master the default so they don't accidentally install the development version. As a result of this feedback, I switched the default branch back to master yesterday.

I'm still not sure master is the best default branch but let's try it for now. I've merged your commits into develop so this PR is closed.

@garrettr garrettr closed this Mar 23, 2015

@garrettr

This comment has been minimized.

Contributor

garrettr commented Mar 23, 2015

I'm still not sure master is the best default branch but let's try it for now.

Here's an example of why I prefer develop to be the default: Github's autoclose syntax only works when commits are merged into the default branch (ref).

@garrettr

This comment has been minimized.

Contributor

garrettr commented Mar 23, 2015

After reading some more Github docs, I realized that develop really should be the default branch. Github assumes that "default branch" = "branch code gets merged in to", which is develop according to our current branching model.

When the default branch is master, then the following undesirable things happen (there are probably even more than we just haven't encountered yet):

  1. Default branch for all pull requests is master, which is always wrong. People usually won't remember to change the target branch before submitting a PR, even when we document it, and there's no way to change the target branch once you've submitted a PR. You just have to close it and create a new one with the correct target branch. This will create tons of PR noise, wasted time, and frustration.
  2. The super-convenient auto-close syntax won't work.

install.md actually recommends checking out the signed tag for a specific release before installation rather than the master branch. This is actually better, because we can't sign a branch (well we could sign every commit but don't get me started) but we do cryptographically sign our release tags (very, very carefully). This is the only way for an installation to ensure that the repo they received is the one we intended, and not a malicious copy provided by a compromised Github or network connection. So installation best practice should be to checkout (and verify) signed tags, and not to rely on the head of master. If that's the case, then there's no argument for making master the default branch and we should stick with develop.

garrettr added a commit that referenced this pull request Mar 23, 2015

@conorsch

This comment has been minimized.

Contributor

conorsch commented Mar 23, 2015

This absolutely should have been submitted to develop, it was an oversight on my part that it targeted master. Agreed on keeping develop the default!

garrettr added a commit that referenced this pull request Apr 28, 2015

garrettr added a commit that referenced this pull request Apr 28, 2015

garrettr added a commit that referenced this pull request Apr 28, 2015

ghost pushed a commit to DC4EC4F68F5C9F5B/securedrop-docs that referenced this pull request Dec 18, 2015

ghost pushed a commit to DC4EC4F68F5C9F5B/securedrop-docs that referenced this pull request Dec 18, 2015

ghost pushed a commit to DC4EC4F68F5C9F5B/securedrop-docs that referenced this pull request Dec 18, 2015

ghost pushed a commit to DC4EC4F68F5C9F5B/securedrop-docs that referenced this pull request Dec 18, 2015

ghost pushed a commit to DC4EC4F68F5C9F5B/securedrop-docs that referenced this pull request Dec 18, 2015

ghost pushed a commit to DC4EC4F68F5C9F5B/securedrop-docs that referenced this pull request Dec 18, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment