Consul Version Fact #209
Consul Version Fact #209
Conversation
| Facter.add(:consul_version) do | ||
| confine :kernel => 'Linux' | ||
| setcode do | ||
| Facter::Core::Execution.exec('consul --version | head -1 | cut -f2 -d\ | cut -c2-') |
There was a problem hiding this comment.
Can you do the raw exec and do the string manipulation in ruby? Same with powershell.
There was a problem hiding this comment.
That's a very good idea, can't believe I didn't think of it rather! Will get back to this.
|
Add a test please for both linux and windows. Here is a similar example if you have not done so before: |
|
Okay, test written, let me know if there's any improvements to be had? |
| version = Facter::Util::Resolution.exec('consul --version') | ||
| version = version.lines.first.split[1].tr('v','') | ||
| elsif Facter.value(:kernel) == 'windows' | ||
| version = Facter::Util::Resolution.exec('"C:\\Program Files\\Consul\\consul.exe" --version') |
There was a problem hiding this comment.
Is there anyway to make this configurable as we can't be certain that this is actually where consul is installed on Windows. For instance we install to C:\Consul not Program Files.
There was a problem hiding this comment.
Agree, not optimal. Since this module doesn't yet support Windows, maybe best to remove Windows support in the fact? Only reason it's there currently is because I/we use Consul on Windows but use a separate module to install/manage it.
There was a problem hiding this comment.
If there is no easy way to parameterize it then yea might want to remove it. My company is working on the Windows support for this module and are baking it in our preprod environments currently so hopefully we'll have that merged in soon.
There was a problem hiding this comment.
Don't think it would be possible to parameterize it, as facts run on the agent prior to catalogue compilation, so don't think they have access to any of the puppet parameters. Would probably have to ensure consul was on the Windows path once support is added.
Also, I wrote the windows puppet module for my companies use of Consul on Windows, depends on NSSM. Happy to contribute to the work you're doing if you're willing to put it in an open repo somewhere?
|
Removed Windows for now, will retain that on my own branch for our use. |
| allow(Facter.fact(:kernel)).to receive(:value).and_return("Linux") | ||
| allow(Facter::Util::Resolution).to receive(:exec).with('consul --version'). | ||
| and_return(consul_version_output) | ||
| expect(Facter.fact(:consul_version).value).to match(/\d+\.\d+\.\d+/) |
There was a problem hiding this comment.
This is fine, but can you add a really strong expect fact to eql '0.6.0' so I don't have to trust the regex logic? With such a controlled input, we should be able to say exactly what the output was, and not worry about spaces or anything extra.
There was a problem hiding this comment.
Bump on this one request, the only blocker to a merge.
There was a problem hiding this comment.
Been hectic, apologies for the delay.
|
What is the expected output of this fact when consul isn't installed? |
|
Looks great! Thanks. |
Bit rough and ready, improvements welcome. Useful to see if all your servers have moved versions when performing an upgrade.