Skip to content
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

Take spaces out of IDs #120

Closed
gravyboat opened this issue Apr 9, 2015 · 13 comments
Closed

Take spaces out of IDs #120

gravyboat opened this issue Apr 9, 2015 · 13 comments

Comments

@gravyboat
Copy link
Contributor

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
Copy link
Contributor

timoguin commented Apr 9, 2015

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

@gravyboat
Copy link
Contributor Author

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

@nmadhok
Copy link
Member

nmadhok commented Apr 10, 2015

@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
Copy link
Contributor Author

@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
Copy link
Member

nmadhok commented Apr 10, 2015

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
Copy link
Contributor Author

@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
Copy link
Member

nmadhok commented Apr 10, 2015

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

@gravyboat
Copy link
Contributor Author

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

@nmadhok
Copy link
Member

nmadhok commented Apr 10, 2015

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

@gravyboat
Copy link
Contributor Author

@nmadhok Yup, PR is #121

@iggy
Copy link
Contributor

iggy commented Apr 10, 2015

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
Copy link
Member

moreda commented Apr 10, 2015

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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants