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

Namespace Pillar Package Table #30

Closed
wants to merge 1 commit into from
Closed

Namespace Pillar Package Table #30

wants to merge 1 commit into from

Conversation

raizyr
Copy link
Contributor

@raizyr raizyr commented Mar 19, 2014

It seems wrong to me that a formula specific package_table should be in the pillar root level. This pull request moves it to live with the rest of the pillar data expected by the formula in pillar['salt']. Additionally, I removed the if/elif statement in favor of pillar.get, this will provide simple fallback to the hardcoded package_table if the os doesn't exist in the pillar package_table.

salt-formula specific portion of pillar.  Fall back to using the hard
coded package_table if the os isn't in pillar's table
@wwentland
Copy link
Contributor

To be honest I find the package_table in pillar to be quite unconventional too and would have rather expected this to be handled like in other formulas by defining various dictionaries/maps in map.jinja that hold the package data and that will use merge=salt['pillar.get']('salt:lookup') or variations thereof.

Would you, @raizyr, be interested in changing the code in this PR to adhere more closely to the conventions defined in http://docs.saltstack.com/en/latest/topics/development/conventions/formulas.html ? It would also require rebasing against master.

There might, however, be a specific reason for this design that escapes me right now.

@whiteinge
Copy link
Contributor

I second @BABILEN 's suggestion to follow other formula conventions for this with salt:lookup.

@gravyboat
Copy link
Contributor

@raizyr Are you planning on making the changes with salt:lookup? Otherwise I'd like to close this PR.

@aboe76 aboe76 mentioned this pull request Dec 31, 2014
@aboe76
Copy link
Member

aboe76 commented Dec 31, 2014

@raizyr please look at #81 if this better?

@aboe76
Copy link
Member

aboe76 commented Feb 5, 2015

@raizyr and @nmadhok can this pull request in because #81 got merged.

@whiteinge whiteinge closed this Feb 5, 2015
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

Successfully merging this pull request may close these issues.

5 participants