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.

Sign up for GitHub

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

Jump to bottom

Create new structured facts for gluster_peers and gluster_volumes #186

Merged
merged 1 commit into from
Jul 19, 2019
Merged

Create new structured facts for gluster_peers and gluster_volumes #186

merged 1 commit into from
Jul 19, 2019

Conversation

tparkercbn
Copy link
Contributor

@tparkercbn tparkercbn commented Nov 24, 2018 •
edited by alexjfisher
Loading

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
Copy link
Contributor Author

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

@bastelfreak
Copy link
Member

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

@bastelfreak bastelfreak added enhancement New feature or request needs-tests labels Nov 26, 2018
@tparkercbn
Copy link
Contributor Author

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

@tparkercbn
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

Thanks! That should help a ton!

@tparkercbn
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

tparkercbn commented Nov 28, 2018 via email

@alexjfisher alexjfisher changed the title Created new structured facts for gluster_peers and gluster_volumes us… WIP: Create new structured facts for gluster_peers and gluster_volumes Nov 28, 2018
@alexjfisher alexjfisher changed the title WIP: Create new structured facts for gluster_peers and gluster_volumes WIP Create new structured facts for gluster_peers and gluster_volumes Nov 28, 2018
ekohl
ekohl reviewed Dec 5, 2018
View reviewed changes
spec/unit/facter/util/fact_gluster_peers_spec.rb Outdated
}
end

gluster_peers = {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can specify hashes inline:

{
  "peer2" => {
    "uuid" => "7d1148a2-f19e-4f18-818f-3396ddf38c30",
      "connected" => 1,
      "state" => 3,
      "status" => "Peer in Cluster",
  },
  "peer3" => {
    "uuid" => "35f53c52-83dc-4100-a1f7-4a7cdeee074d",
    "connected" => 1,
    "state" => 3,
    "status" => "Peer in Cluster",
  },
}

Possibly Rubocop has something about this, but the idea is clear I hope ;)

@tparkercbn
Copy link
Contributor Author

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

@svenbs svenbs mentioned this pull request Jun 12, 2019
Open
tryfunc
tryfunc suggested changes Jul 18, 2019
View reviewed changes
Copy link

@tryfunc tryfunc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @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.txt
  2. 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.

@tparkercbn
Copy link
Contributor Author

tparkercbn commented Jul 18, 2019 via email

@tryfunc
Copy link

tryfunc commented Jul 18, 2019

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
Copy link
Contributor Author

tparkercbn commented Jul 18, 2019 via email

@tryfunc
Copy link

tryfunc commented Jul 18, 2019

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

gluster_spec.rb.txt

@tparkercbn
Copy link
Contributor Author

tparkercbn commented Jul 18, 2019 via email

@bastelfreak
Copy link
Member

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/

@tparkercbn tparkercbn changed the title WIP Create new structured facts for gluster_peers and gluster_volumes Create new structured facts for gluster_peers and gluster_volumes Jul 18, 2019
@tparkercbn
Created new structured facts for gluster_peers and gluster_volumes us…
a56abdf 
…ing the --xml cli modifier.

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

Special thanks to @tryfunc for the help with the spec tests.
@bastelfreak bastelfreak merged commit b3fe6ba into voxpupuli:master Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ekohl ekohl ekohl left review comments

@tryfunc tryfunc tryfunc requested changes

Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
5 participants
@tparkercbn @bastelfreak @alexjfisher @tryfunc @ekohl