search

voxpupuli / puppet-gluster

Create and manage Gluster pools, volumes, and mounts
https://forge.puppet.com/puppet/gluster
MIT License
16 stars 72 forks source link

Create new structured facts for `gluster_peers` and `gluster_volumes` #186

Closed tparkercbn closed 5 years ago

tparkercbn commented 6 years ago

Created new structured facts for gluster_peers and gluster_volumes using the --xml cli modifier.

All legacy facts are unchanged to not cause any breaking changes to people who may be using them

Pull Request (PR) description

Created new structured facts for gluster_peers and gluster_volumes that fix the problems caused by comma separated data structures containing comma separated values. It also fixes the gluster_volume_ports bug when the volume names line wrap

This Pull Request (PR) fixes the following issues

Fixes #165 Fixes #53 Fixes #33

tparkercbn commented 6 years ago

This pull request replaces my previous one that ended up being a complete mess of commits. I'm apparently not great with Git :)

bastelfreak commented 5 years ago

Hi @tparkercbn, thanks for the PR. Can you please add a test test for the changes?

tparkercbn commented 5 years ago

I have no idea how to do that :) Let me see what I can figure out.

tparkercbn commented 5 years ago

Are there any current tests for the output of the facts? My code makes no changes at all to the functionality of the module itself other than fixing the comma separated values bug in the volume options

alexjfisher commented 5 years ago

@tparkercbn Would some examples help?

https://github.com/voxpupuli/puppet-nginx/blob/master/spec/unit/facter/util/fact_nginx_version_spec.rb https://github.com/voxpupuli/puppet-mongodb/blob/master/spec/unit/mongodb_version_spec.rb

If you're really stuck, let us know.

tparkercbn commented 5 years ago

Thanks! That should help a ton!

tparkercbn commented 5 years ago

I am going to be on a business trip for a few weeks and cannot get the spec tests running on my laptop (X crashes when I start the container). I will try to get a test server running when I have some time but the test suite may take a but more time than I hoped.

bastelfreak commented 5 years ago

Docker + systemd + Xorg are sometimes complicated. It is totally fine to just push changes and watch the outcome on travis. It will execute the tests for you. You just need to squash the git history afterwards.

tparkercbn commented 5 years ago

Ok cool. I was hoping to be able to learn a bit more about the tests before making my code public :D but that's what I will do.

Thanks!

Get Outlook for Androidhttps://aka.ms/ghei36

From: Tim Meusel notifications@github.com Sent: Wednesday, November 28, 2018 2:15:06 PM To: voxpupuli/puppet-gluster Cc: Tom Parker; Mention Subject: Re: [voxpupuli/puppet-gluster] Created new structured facts for gluster_peers and gluster_volumes us… (#186)

Docker + systemd + Xorg are sometimes complicated. It is totally fine to just push changes and watch the outcome on travis. It will execute the tests for you. You just need to squash the git history afterwards.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/voxpupuli/puppet-gluster/pull/186#issuecomment-442589109, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AFOC9iFiJ2erEwmDPGJCdAVuM_vqlBWdks5uzu7KgaJpZM4YxQfa.

tparkercbn commented 5 years ago

Sorry. I haven't forgotten about this but work and travel has had me up to my eyeballs.

For some reason it's not finding the data that I am passing to validate. Is there someone who can take a look and make some suggestions?

Thanks!

Tom

tparkercbn commented 5 years ago

This is awesome! Thanks 😊

I applied the patches and pushed the change but we are failing the rubocop checks in travis-ci

I’m not sure how to properly give you credit for this work. Maybe someone else on the team knows

Tom

From: tryfunc notifications@github.com Sent: Thursday, July 18, 2019 1:17 AM To: voxpupuli/puppet-gluster puppet-gluster@noreply.github.com Cc: Tom Parker tparker@cbnco.com; Mention mention@noreply.github.com Subject: Re: [voxpupuli/puppet-gluster] WIP Create new structured facts for gluster_peers and gluster_volumes (#186)

@tryfunc requested changes on this pull request.

Hi @tparkercbnhttps://github.com/tparkercbn, have a couple of patches.

  1. For lib/facter/gluster.rb. I think it is better to take out the facts the "gluster_peers" and the "gluster_volumes" from the "unless". At a minimum, gluster_peers may contain data when the volume is missing.
  2. I corrected the fact_gluster_peers_spec.rb. (coverage 94.12%). It is performed successfully only if the first patch is applied (see above 1.) It would also be more logical if the file were located along the path spec/unit/lib/facter/gluster_spec.rb.

Putting patches on both changes:

  1. gluster.rb.patch.txthttps://github.com/voxpupuli/puppet-gluster/files/3404824/gluster.rb.patch.txt
  2. gluster_spec.rb.patch.txthttps://github.com/voxpupuli/puppet-gluster/files/3404825/gluster_spec.rb.patch.txt

You can apply fixes by placing them in the root of the repository and running the commands:

git apply gluster.rb.patch.txt

git apply gluster_spec.rb.patch.txt

I would like to send a "pull request" to your "pull request", but it is not clear how to do this.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/voxpupuli/puppet-gluster/pull/186?email_source=notifications&email_token=ABJYF5SEJ3G5LPVKLHVDV4LP774FXA5CNFSM4GGFA7NKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB6Z2YPA#pullrequestreview-263433276, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABJYF5QO6YZR3KXLONU2T2DP774FXANCNFSM4GGFA7NA.

tryfunc commented 5 years ago

Cool! Thank! One caveat, the gluster.rb.patch.txt and gluster_spec.rb.patch.txt patch files are not needed in the repository. It would be better to remove them.

Oh, one test failed. Not quite the correct syntax. Now I will fix it.

tparkercbn commented 5 years ago

Yeah, I just stashed them there and then forgot to remove before commit. Still on my first coffee 😊

I was going to try to cleanup the indenting but you are probably faster 😊

Tom

From: tryfunc notifications@github.com Sent: Thursday, July 18, 2019 11:31 AM To: voxpupuli/puppet-gluster puppet-gluster@noreply.github.com Cc: Tom Parker tparker@cbnco.com; Mention mention@noreply.github.com Subject: Re: [voxpupuli/puppet-gluster] WIP Create new structured facts for gluster_peers and gluster_volumes (#186)

Cool! Thank! One caveat, the gluster.rb.patch.txt and gluster_spec.rb.patch.txt patch files are not needed in the repository. It would be better to remove them.

Oh, one test failed. Not quite the correct syntax. Now I will fix it.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/voxpupuli/puppet-gluster/pull/186?email_source=notifications&email_token=ABJYF5R3PELLR5TTVMA7EVDQACEDPA5CNFSM4GGFA7NKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2I3UMA#issuecomment-512866864, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABJYF5UH74M5D6EBPV23QKTQACEDPANCNFSM4GGFA7NA.

tryfunc commented 5 years ago

Attached is the revised version of gluster _spec.rb. Checked several times - everything is ok.

gluster_spec.rb.txt

tparkercbn commented 5 years ago

Thank you.

All tests look good!

Now I will have to figure out how to rebase this all on master so it’s not a mess of commits 😊

I really appreciate your help on this. I am still very new to rspec testing

Tom

From: tryfunc notifications@github.com Sent: Thursday, July 18, 2019 2:25 PM To: voxpupuli/puppet-gluster puppet-gluster@noreply.github.com Cc: Tom Parker tparker@cbnco.com; Mention mention@noreply.github.com Subject: Re: [voxpupuli/puppet-gluster] WIP Create new structured facts for gluster_peers and gluster_volumes (#186)

Attached is the revised version of gluster _spec.rb. Checked several times - everything is ok.

gluster_spec.rb.txthttps://github.com/voxpupuli/puppet-gluster/files/3407901/gluster_spec.rb.txt

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/voxpupuli/puppet-gluster/pull/186?email_source=notifications&email_token=ABJYF5X7ZDQRYSK6RILCSHLQACYRPA5CNFSM4GGFA7NKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2JLR5Y#issuecomment-512932087, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ABJYF5RGI7YWGAGFSSPIID3QACYRPANCNFSM4GGFA7NA.

bastelfreak commented 5 years ago

Hey @tparkercbn. If you need some help with rebasing you can ping us in our IRC channel #voxpupuli on freenode or on https://slack.puppet.com/