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

cmd_yaml pillar can't be used on a per minion basis #2276

Closed
abh opened this Issue Oct 18, 2012 · 14 comments

Comments

Projects
None yet
7 participants
@abh
Contributor

abh commented Oct 18, 2012

It seems like a cmd_yaml pillar can only setup "global" data as it doesn't get the minion id passed as a parameter like the ext_nodes command does.

(Unless I missed something in which case maybe the documentation could be expanded?) :-)

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Oct 18, 2012

Member

Heh, good call, the cmd_yaml is primarily a proof of concept, I will look into this.
Do you have a specific use case to ref?

Member

thatch45 commented Oct 18, 2012

Heh, good call, the cmd_yaml is primarily a proof of concept, I will look into this.
Do you have a specific use case to ref?

@abh

This comment has been minimized.

Show comment
Hide comment
@abh

abh Oct 18, 2012

Contributor

I have a couple dozen hosts that need just a few configuration keys each (IP addresses to bind on for a couple services). Editing pillar/top.sls and pillar/hostname.sls (or whatever) just for that feels like a lot of overhead.

With an external command I can put it in one file and have my external command do a tiny bit of filtering and also programmatically provide defaults (some data I can lookup in DNS) etc.

Contributor

abh commented Oct 18, 2012

I have a couple dozen hosts that need just a few configuration keys each (IP addresses to bind on for a couple services). Editing pillar/top.sls and pillar/hostname.sls (or whatever) just for that feels like a lot of overhead.

With an external command I can put it in one file and have my external command do a tiny bit of filtering and also programmatically provide defaults (some data I can lookup in DNS) etc.

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Oct 18, 2012

Member

Thanks :)
This is a crazy easy addition, the hard part of deciding what all should be passed in and how.

Member

thatch45 commented Oct 18, 2012

Thanks :)
This is a crazy easy addition, the hard part of deciding what all should be passed in and how.

@ghost ghost assigned thatch45 Nov 8, 2012

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Nov 11, 2012

Member

I am leaning towards not changing this. The main reason for the external pillar system to to deliver the capability to get this data to the pillar system.
For now I am going to move this to approved for the future pending discussion.

Member

thatch45 commented Nov 11, 2012

I am leaning towards not changing this. The main reason for the external pillar system to to deliver the capability to get this data to the pillar system.
For now I am going to move this to approved for the future pending discussion.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 22, 2013

Contributor

I don't understand why you are opposed to changing this. ext_nodes gets the node ID, so why shouldn't cmd_yaml? https://groups.google.com/d/msg/salt-users/R_jgNdYDPk0/u2xAwdWiu3UJ has the patch, this is all that's required, won't break existing code, and I cannot see a reason why that wouldn't be enough — and if it isn't, then maybe we should talk about extending the interface?

Contributor

madduck commented Jan 22, 2013

I don't understand why you are opposed to changing this. ext_nodes gets the node ID, so why shouldn't cmd_yaml? https://groups.google.com/d/msg/salt-users/R_jgNdYDPk0/u2xAwdWiu3UJ has the patch, this is all that's required, won't break existing code, and I cannot see a reason why that wouldn't be enough — and if it isn't, then maybe we should talk about extending the interface?

@thatch45

This comment has been minimized.

Show comment
Hide comment
@thatch45

thatch45 Jan 22, 2013

Member

I will get that patch in!

Member

thatch45 commented Jan 22, 2013

I will get that patch in!

@thatch45 thatch45 closed this in 85e352e Feb 1, 2013

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Feb 1, 2013

Contributor

The docs at https://salt.readthedocs.org/en/latest/ref/configuration/master.html#ext-pillar use /bin/cat with cmd_yaml.py; this will now no longer work. I suggest to replace it with

cmd_yaml: /path/to/script/returning-yaml-for-hostname
Contributor

madduck commented Feb 1, 2013

The docs at https://salt.readthedocs.org/en/latest/ref/configuration/master.html#ext-pillar use /bin/cat with cmd_yaml.py; this will now no longer work. I suggest to replace it with

cmd_yaml: /path/to/script/returning-yaml-for-hostname
@m-o-e

This comment has been minimized.

Show comment
Hide comment
@m-o-e

m-o-e May 29, 2013

Please re-open this ticket. The above fix was reverted in 4ec6722.

m-o-e commented May 29, 2013

Please re-open this ticket. The above fix was reverted in 4ec6722.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi May 29, 2013

Collaborator

Done.

Collaborator

basepi commented May 29, 2013

Done.

@basepi basepi reopened this May 29, 2013

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Aug 27, 2013

Contributor

@thatch45 @basepi: We either have to make an incompatible change to cmd_yaml, or let this issue go. However, since Pillar data are specific to a node, and right now the command has no way to figure out which minion's Pillar data to serve, the current interface is rather useless. Correct me if I am wrong.

If you want, then I can create a patch that catches run-time exceptions from cmd.run and falls back to the old behaviour. Though I really don't think we need to be this careful, I cannot imagine any real use-case with the status quo.

Contributor

madduck commented Aug 27, 2013

@thatch45 @basepi: We either have to make an incompatible change to cmd_yaml, or let this issue go. However, since Pillar data are specific to a node, and right now the command has no way to figure out which minion's Pillar data to serve, the current interface is rather useless. Correct me if I am wrong.

If you want, then I can create a patch that catches run-time exceptions from cmd.run and falls back to the old behaviour. Though I really don't think we need to be this careful, I cannot imagine any real use-case with the status quo.

@mbarrien

This comment has been minimized.

Show comment
Hide comment
@mbarrien

mbarrien Jan 21, 2014

Contributor

What about adding a **kwargs argument to the ext_pillar for cmd_yaml/cmd_json? Something like a "template: jinja" or an even simpler "pass_args: True" could enable this behavior, and by default it is disabled, allowing backward compatibility.

Even adding this simple feature will remove the need for me to have 3 custom external pillars.

Contributor

mbarrien commented Jan 21, 2014

What about adding a **kwargs argument to the ext_pillar for cmd_yaml/cmd_json? Something like a "template: jinja" or an even simpler "pass_args: True" could enable this behavior, and by default it is disabled, allowing backward compatibility.

Even adding this simple feature will remove the need for me to have 3 custom external pillars.

@madduck

This comment has been minimized.

Show comment
Hide comment
@madduck

madduck Jan 22, 2014

Contributor

There is no way to parse **kwargs to the external pillar without breaking the existing interface. I think the simplest backwards-compatible solution would be to put the minion ID into an environment variable.

Contributor

madduck commented Jan 22, 2014

There is no way to parse **kwargs to the external pillar without breaking the existing interface. I think the simplest backwards-compatible solution would be to put the minion ID into an environment variable.

@Grokzen

This comment has been minimized.

Show comment
Hide comment
@Grokzen

Grokzen Jun 5, 2015

This could be closed based on this PR #22461 that is now in develop branch.

Grokzen commented Jun 5, 2015

This could be closed based on this PR #22461 that is now in develop branch.

@basepi

This comment has been minimized.

Show comment
Hide comment
@basepi

basepi Jun 5, 2015

Collaborator

Awesome, thanks!

Collaborator

basepi commented Jun 5, 2015

Awesome, thanks!

@basepi basepi closed this Jun 5, 2015

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