Parse XML output of gluster for retrieving facts #118
Conversation
coder-hugo
force-pushed
the
feature/minor-fixes
branch
from
bfbf0f5 to
653f129
Compare
lib/facter/gluster.rb
Outdated
| if volume_options | ||
| volume_options.each do |vol, opts| | ||
| Facter.add("gluster_volume_#{vol}_options".to_sym) do | ||
| setcode do | ||
| opts.join(',') | ||
| opts.join(';') |
There was a problem hiding this comment.
Do you think this counts as a breaking change? ie Outside its use in this module, users might be using this fact in their own profiles?? Now that we've dropped puppet 3 support, all users should have a version of facter that supports structured-facts This means the fact could be changed to just return the array - getting rid of the join and corresponding split?
There was a problem hiding this comment.
By using ; instead of , as separator for the volume options I tried to fix this error as less invasive as possible. If it's now possible to use sturctured-facts in this module I can adapt the code to do this. In this case I'd split this PR into 2. One that adds the code for using the xml output of gluster for retrieving the facts. And the other one that converts the volume fact from a string to an array. This will make it also easier to give the PRs a better name 😉
There was a problem hiding this comment.
+1 for splitting into 2 PRs if possible. That will make it easier to get reviews and merged. And if you're feeling really bored/generous, feel free to completely refactor the hideousness going on in volume.pp in a 3rd :)
|
@coder-hugo Thanks for your work. Could you rename the PR to something that would make sense in the Changelog? (Besides I'm not sure how 'minor' I'd call 548baa7 !) Looking at the error message...
That's from calling parseyaml on invalid yaml the manifest builds. WTF is going on in that manifest!!?? ;) eg puppet-gluster/manifests/volume.pp Line 139 in 5c2e385 # now we make some YAML, and then parse that to get a Puppet hash
$yaml = join( regsubst( $vol_opts, ': ', ":\n value: ", 'G'), "\n")
$hoh = parseyaml($yaml)seriously? a yaml string is used as some intermediate data format?? surely there's a better more sane way? |
coder-hugo
force-pushed
the
feature/minor-fixes
branch
from
653f129 to
548baa7
Compare
coder-hugo
changed the title
|
Hi @coder-hugo, could you please rebase and take a look at the comments from @alexjfisher ? |
|
@coder-hugo what did you do to work around this issue? I am currently facing the same issue with auth.allow not been able to set multiple ips. |
|
Dear @coder-hugo, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
|
Dear @coder-hugo, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
1 similar comment
|
Dear @coder-hugo, thanks for the PR! This is pccibot, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase? You can find my sourcecode at voxpupuli/vox-pupuli-tasks |
This PR provides 2 minor fixes. The first will fix #33 by parsing the xml output of the gluster command. The second one uses
;instead of,as separator for the options in thegluster_volume_#{vol}_optionsfact as the comma is already used by some gluster options (e.g.auth.ssl-allow) as separator. Having such an option in use every puppet run will fail with the following error: