Upgrade/Downgrade url method #114
Conversation
| # $ consul -v | ||
| # Consul v0.5.0 | ||
| # Consul Protocol: 2 (Understands back to: 1) | ||
| consul_v = Facter::Util::Resolution.exec("consul -v") |
There was a problem hiding this comment.
This seems like it could be potentially brittle or hard to refactor.
Can you add a test for this to stub out exec with the example that your provided in the comments and assert the desired expected output?
There was a problem hiding this comment.
Def, I can add a test for this.
Also, regarding refactoring, do you think its better to inject those values in a hash or something? So incase in the future we need more than just version or protocol it would be easily manageable? Or did you mean some other way to refactor? I can imagine adding more consul facts in the future.
There was a problem hiding this comment.
I think just normal strings is ok.
I'm mostly concerned about guarding against consul not being available, string parsing, and timeouts. I've been bit by all those things.
There is another thing I would like to suggest in this refactor and that is to make it so it returns "unknown" when not available. This makes the fact easier to use in contexts when strict variables is enabled in puppet.
We do not currently test with strict variables on, but I think we should.
This allows us to upgrade/downgrade consul's if the installation method is URL.