search

saltstack-formulas / salt-formula

Yes, Salt can Salt itself!
http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html
Other
198 stars 421 forks source link

Take spaces out of IDs #120

Closed gravyboat closed 9 years ago

gravyboat commented 9 years ago

Looks like this is most prevalent here: https://github.com/saltstack-formulas/salt-formula/blob/master/salt/ssh.sls (shame on you @timoguin ;) ). We should fix that.

timoguin commented 9 years ago

Well I like the self-documenting IDs, so I disagree. But it's not going to hurt my feelings if it gets changed.

gravyboat commented 9 years ago

I also like the self documentation, it will be an easy fix to just add underscores/dashes.

nmadhok commented 9 years ago

@gravyboat I don't like underscores or dashes in the ID and feel spaces are just fine. Can I close this issue? I think it does not break any functionality and is more readable.

gravyboat commented 9 years ago

@nmadhok I don't think so, I honestly don't care what people do personally, but it should be standardized across the repo, and currently the standard is dashes for this repo. That's really my only reasoning behind opening this in the first place.

nmadhok commented 9 years ago

We need to refactor all formulas in that case if a standard needs to be put in place. Is there a good reason you think we shouldn't use spaces?

gravyboat commented 9 years ago

@nmadhok Not particularly, just that it doesn't jive with the rest of this formula specifically. I don't think we should go through changing this in all the docs/formulas as it's too much work with too little benefit. It also helps users pick their own preferred method as I doubt we'll be able to reach consensus on a standard.

nmadhok commented 9 years ago

Agreed. I thought you were talking about changing it in all formulas. Sorry my bad

gravyboat commented 9 years ago

@nmadhok No worries, sorry for not being clear enough. It was just this particular state since the others in the formula use dashes!

nmadhok commented 9 years ago

@gravyboat I hadn't noticed that. Good call!

gravyboat commented 9 years ago

@nmadhok Yup, PR is #121

iggy commented 9 years ago

My big issue with spaces is it starts to look horrendous in requisite listings. But yeah... we're not going to get buy in for every formula. Best we can do is make sure each formula is consistent.

    - require:
      - pkg: ensure salt-ssh is installed

I think anyone new to Salt is going to look at that and be like "wtf".

moreda commented 9 years ago

Hi, Just my 2 cents: in general I'd like to avoid the usage of an ID different than the name parameter (yes, exceptions exist and hence the usual light approach of adding prefixes or suffixes using "_" or "-").

There are some good reason to enforce unique IDs: a "creative self-documenting usage", even if it's for clarification purposes (why not a comment block?) could mean race conditions and unexpected results. For example, if you declare a file as absent and in a different part of the tree you declare it as managed using different IDs in each one of them (ditto for users, packages, etc.) you can have some inconsistent result and Saltstack it's going to say nothing.

Take in account as well that even if you can mimic an Ansible playbook ;-) and Saltstack is flexible enough to do that and worse (Gosh, how my eyes bleed when I see "- order: n" sometimes! :-)) sometimes that's not the best course of action (i.e. ignoring safeguards like unique ID rules or the powerful semantics of the "Require" mechanism that allows potential parallel processing).

What @iggy just commented reinforces my opinion... ;-)

Again: my 2 cents with a huge IMHO :-) Cheers!

gravyboat commented 9 years ago

PR is merged, closing this. Discussion can continue if you guys want, just no reason to leave it open.