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

Added pillar:// scheme to file.managed. #6508

Merged
merged 5 commits into from
Aug 6, 2013

Conversation

rca
Copy link
Contributor

@rca rca commented Aug 3, 2013

It's very difficult to use the contents option in file.managed with pillar data and I came up with a quick way to get this functionality into file.managed by passing in a source with the pillar:// scheme followed by a path usable by pillar.get. This works amazingly well and looks to me like a natural fit to the way file.managed operates. For example:

/etc/myservice.conf:
  file.managed:
    - source: pillar://services:myservice:conf

Please share your thoughts and if there is a better place to check for the pillar scheme. The current implementation is a bit hacked in and needs tests.

Thanks.

This allows pillar data to be placed into a file without having to worry
about indentation in a sls file.
@terminalmage
Copy link
Contributor

I would be hesitant to add this just to the file state, if we had something like this then it should be implemented at the level of the fileclient. Otherwise, the other states that use file URIs won't have support for it, which is unintuitive and would confuse users.

I'm interested to hear what the other core devs think, however.

@rca
Copy link
Contributor Author

rca commented Aug 3, 2013

That's a great point. I looked through the code and wasn't quite sure how to fit it into fileclient. Any pointers on where it should go?

@terminalmage
Copy link
Contributor

Not sure, I've done some work in the fileclient before but I'd have to take some time to see how it would fit in.

Overall, I love the idea, because the current way of accomplishing something like this is to first create a jinja template file containing a reference to the pillar that you want to use, and then make a file.managed state like the following:

/etc/foo.conf:
  file.managed:
    - source: salt://path/to/jinja/template
    - template: jinja

This is quite hacky and means you need to have a different template file for each pillar that you wish to reference in this way. So I love the idea of creating an interface to pillar.

@thatch45
Copy link
Contributor

thatch45 commented Aug 4, 2013

Wow @rca, this is simple! I don't think that this will be able to work in other states quite the same way since only file.managed supports contents. Also I don't think that this can work in the fileclient, again it does not translate cleanly.
So the only problem is that this protocol only works in a single place, and may be confusing, but I also don't see it applying elsewhere, thoughts?

@UtahDave
Copy link
Contributor

UtahDave commented Aug 4, 2013

This has been a highly requested feature. I agree that it's mostly useful
with file.managed, so I'm fine with this implementation. I'd like to see
this clearly documented to avoid confusion.
On Aug 4, 2013 12:00 PM, "Thomas S Hatch" notifications@github.com wrote:

Wow @rca https://github.com/rca, this is simple! I don't think that
this will be able to work in other states quite the same way since only
file.managed supports contents. Also I don't think that this can work in
the fileclient, again it does not translate cleanly.
So the only problem is that this protocol only works in a single place,
and may be confusing, but I also don't see it applying elsewhere, thoughts?


Reply to this email directly or view it on GitHubhttps://github.com//pull/6508#issuecomment-22075836
.

Per discussion in issue saltstack#6508 applying a `pillar://` scheme to the
`source` parameter will be confusing because there is no parity with
other locations that use `source` and the most useful location for this
is the `file.managed` state.  In order to alleviate the problem, apply
the logic to the `contents` parameter, which is only supported in
`file.managed`.
@rca
Copy link
Contributor Author

rca commented Aug 4, 2013

How about moving pillar:// scheme parsing to the contents parameter, which only applies to file.managed and will eliminate confusion with other places that used source ? Please see patch above.

@rca
Copy link
Contributor Author

rca commented Aug 4, 2013

Example usage now is:

/etc/myservice.conf:
  file.managed:
    - contents: pillar://services:myservice:conf

@thatch45
Copy link
Contributor

thatch45 commented Aug 4, 2013

How about we add a pillar argument to file.managed? So that we don't have to confuse a new URL? So it would be like this:

/path/to/file:
  file.managed:
    - pillar: foo:bar

@terminalmage
Copy link
Contributor

Instead of calling it just pillar, what about calling it contents_pillar. Sounds more descriptive that way.

@thatch45
Copy link
Contributor

thatch45 commented Aug 4, 2013

Yes. I was thinking the same

This builds off of pull req saltstack#6508, but discards the new URI scheme for a
dedicated argument to get the file contents from pillar.
@terminalmage
Copy link
Contributor

@rca I just submitted a pull request to your fork. If this works for you, feel free to merge it and this pull should be ready to merge.

@rca
Copy link
Contributor Author

rca commented Aug 5, 2013

@terminalmage: merged and added some tests.

@terminalmage
Copy link
Contributor

Sweet! It looks good, but I'd like to test it on Windows as well before it is merged. I'll test it on Monday morning.

@terminalmage
Copy link
Contributor

This works on Windows, and can now be merged into develop.

thatch45 added a commit that referenced this pull request Aug 6, 2013
@thatch45 thatch45 merged commit d7aca29 into saltstack:develop Aug 6, 2013
@terminalmage
Copy link
Contributor

Thanks @rca for the suggestion, and double thanks for the test!

@rca rca deleted the file_managed_pillar_source_scheme branch August 6, 2013 05:51
@rca
Copy link
Contributor Author

rca commented Aug 6, 2013

No problem, glad I can help!

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.

4 participants