Update state files for Salt best practices

[aneeshusa]

Big Servo + Saltstack fan, just wanted to make these states look a little neater and shorter.

• Some of the values in common/map.jinja (the hosts and ssh_users keys) are much better suited to living in the pillar from my point of view, but are also ok in the state tree.
• Is servo-mac3 supposed to be servo-mac2? Wasn't sure if it was an oversight or on purpose.
• It looks like you're already using the pillar for other values, so I wasn't able to test this thoroughly without appropriate fake values - test this out before merging.

Let me know if you have any questions about the changes.

[Manishearth]

r? @edunham @larsbergstrom

We have three mac builders, though not all run the same jobs. The SSH pubkeys are okay public IMO; pillar should be bare minimum.

[edunham]

Reviewed 37 of 39 files at r1, 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

^{builder.sls, line 0 [r2] (raw file):}
I wonder why we had an empty file here. https://github.com/servo/saltfs/blob/master/builder.sls from initial commit... @metajack, thoughts?

---

^{common.sls, line 0 [r2] (raw file):}
Became common/init, with info like ssh users factored out into common/map.jinja.

---

Comments from the review on Reviewable.io

[edunham]

Thank you for the PR! The cleanup/best practices looks good to me. Any remaining errors are more subtle than I can detect on a read through 👍

[aneeshusa]

Yeah, unfortunately I don't have a good way to test the individual targets myself; I'd recommend just doing a dry run highstate (salt-call --local state.highstate test=True) on the various target types from top.sls to make sure everything works before merging. Glad to hear you liked it.

[Manishearth]

Should we be Travising dry runs (with dummy pillar data)? Or would that be hard to set up?

[aneeshusa]

Yeah, that would be a good idea, it's best to cover all the targets and Travis makes it easy. Everything takes only a few minutes to run anyways, and the states are fairly isolated. Using dummy pillar data sounds good, and you'd want to use dry-run mode (test=True) with that.

[Manishearth]

(I don't know how to set that up, so if you're looking for stuff to work on, that's something that would be really great to have!)

[larsbergstrom]

@aneeshusa Thank you so much for this PR! These all look like fantastic clean-ups and will really help us with our future changes.

Once @metajack returns from a business trip this week, we will pore over it in more detail and then probably land and push it out. I hope you don't mind if we reach out to you in the future with our SaltStack questions :-)

[aneeshusa]

@Manishearth it looks like Mac OSX / multi-OS builds on Travis are still in beta, so you'll need to email support to enable it. I'll prep another PR to get set up with multi-os support; let me know once Travis has enabled multi-os builds for the repo, and then I'll push up the branch and let Travis take it for a spin. Apparently Python isn't yet officially supported on Mac OS X so we'll have to do a bit of manual installation, but it shouldn't be too bad. It would be best to get multiple builds working first before merging this PR so it gets tested thoroughly.

@larsbergstrom No problem! I find Saltstack to be a great tool and I'm glad you (and apparently other projects Mozilla is working on, like Let's Encrypt) are using it. I'd be happy to help out in the future, just let me know if you want PRs reviewed or thoughts on how to add a new feature, etc.

[Manishearth]

Emailed. Feel free to PR up a linux version in the meantime, and thanks for the offer! 😄

[Manishearth]

[osx support enabled]

[bors-servo]

☔ The latest upstream changes (presumably #100) made this pull request unmergeable. Please resolve the merge conflicts.

[metajack]

This needs a rebase, but I'm back and have landed the changes that were preventing this from getting merged. So as soon as it's rebased we'll review and merge.

[aneeshusa]

Great, I'll work on the rebase. However, I think it's prudent to get #97 merged first so we can be testing all node types, because this PR touches pretty much all the states and I wouldn't have an easy way of testing them all locally. Letting Travis do that is much simpler. (#97 also needs a rebase and some updates due to the recent changes before getting merged.) Thoughts?

I hope your trip went well :)

[metajack]

Cool. I'm happy to merge #97 first. I left a comment over there.

[aneeshusa]

Hey, sorry about the delay, got a little busy with school starting. It looks like Android on Travis-CI is flaky in general, so I think it would be best to save #97 for later (or have it test everything but Android, I guess). With that in mind, can we take another look at merging this?

[metajack]

Reviewed 3 of 9 files at r3.
Review status: 21 of 27 files reviewed at latest revision, 2 unresolved discussions, all commit checks successful.

---

Comments from the review on Reviewable.io

[metajack]

Aside from the typo breaking this anyway, why can the require_ins be removed?

[metajack]

Looks good to me except for the question above.

[aneeshusa]

watch (and watch_in) include the functionality of require (resp. require_in), so watch adds additional behavior on changes. If you want to trigger a state only when another states changes, there is an onchanges (onchanges_in) requisite for that. Definitely a little confusing, but cleaner once you understand the difference.

Docs: https://docs.saltstack.com/en/latest/ref/states/requisites.html#watch

[metajack]

@bors-servo r+

[bors-servo]

📌 Commit b015a90 has been approved by metajack

[bors-servo]

⌛ Testing commit b015a90 with merge c53ac7b...

[bors-servo]

☀️ Test successful - travis