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

Add puppet_server fact to return agent's server #613

Merged
merged 1 commit into from
Dec 20, 2016

Conversation

reidmv
Copy link
Contributor

@reidmv reidmv commented Jun 29, 2016

It is frequently useful to configure an agent to retrieve a resource
from it's configured master, or make further configuration adjustments
to itself based on what server it's using. Similar to the rationale for
stdlib providing a puppet_vardir fact, this commit adds a puppet_server
fact.

Note that the closest equivalent available today is $settings::server,
which returns only the MASTER's configured server, not the AGENT's. This
makes $settings::server unreliable, and not useful in a multi-master
deployment or a deployment involving a load balancer.

@wilson208
Copy link
Contributor

@reidmv I know this is an old PR, but if you are still interested in getting it merged, could you add some spec tests to accompany this change? If not, close this PR.

Thanks :)

@reidmv
Copy link
Contributor Author

reidmv commented Nov 10, 2016

@wilson208 Ah, yeah, I remember this.

Does your comment indicate that if this submission had tests, it would be merged?

Let's please be explicit on a viable/non-viable decision first regarding the utility/merge-ability of a fact called "puppet_server". After that's been made clear, and if it merits it, we can buff up the PR code quality to standard.

There's no point writing more code until a decision has been made that the fact merits inclusion in stdlib. (do you agree?)

It is frequently useful to configure an agent to retrieve a resource
from it's configured master, or make further configuration adjustments
to itself based on what server it's using. Similar to the rationale for
stdlib providing a puppet_vardir fact, this commit adds a puppet_server
fact.

Note that the closest equivalent available today is $settings::server,
which returns only the MASTER's configured server, not the AGENT's. This
makes $settings::server unreliable, and not useful in a multi-master
deployment or a deployment involving a load balancer.
@reidmv
Copy link
Contributor Author

reidmv commented Nov 10, 2016

@wilson208 I took a look at this and refactored to match the current codebase. As you can see, this fact is functionally identical to two existing facts (as is evidenced by the fact it belongs in the same file as them now), and those two facts do not have spec tests.

It would be nice to add tests but realistically this PR should not be gated on that.

Edit: oh, I think I see the tests now. Strike that last line. Comment though; given the existing test(s), is there really any utility in adding more coverage? It looks like testing for one given value is exactly the same as testing for any other, and we currently only test for one of the two fact values written this way. https://github.com/puppetlabs/puppetlabs-stdlib/blob/master/spec/unit/facter/util/puppet_settings_spec.rb#L21-L35

@wilson208 wilson208 merged commit 7fe6f07 into puppetlabs:master Dec 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants