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.

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

fix a couple of problems with erl_ssl_path fact #606

Closed
wants to merge 0 commits into from

Conversation

costela
Copy link
Contributor

@costela costela commented Sep 5, 2017

Hi there,

This small PR fixes two small issues with the erl_ssl_path fact.
The first is pretty straightforward: avoid the .gsub call if we didn't actually get any data to mangle.
The second is a bit less clear-cut: drop the use of .with_env while calling erl. Newer versions of facter (>=3) AFAIK stopped supporting it (compare 2.x docs with 3.x docs) and it was unclear why we needed it in the first place. If I'm missing something and it is needed in some case, I'd be happy to find another workaround.

Cheers

@wyardley
Copy link
Contributor

wyardley commented Sep 5, 2017

@costela couple things:

  1. I should have asked for tests on the first go-round with this new fact. The test should be in spec/unit/facter/util/fact_erl_ssl_path.rb. Let me know if you need more help / guidance on this part. Also, until we get modulesync run (as part of the module moving to Voxpupuli), please manually run rubocop on the test to make sure you don't introduce any new rubocop warnings.
  2. Voxpupuli generally likes the commits to be squashed (so rebase, fixup the other commits, and then force-push the branch).

@wyardley wyardley added needs-squash needs-tests needs-work not ready to merge just yet and removed needs-work not ready to merge just yet labels Sep 5, 2017
@wyardley
Copy link
Contributor

wyardley commented Sep 5, 2017

@costela ps - Feel free to chat in #voxpupuli on irc.freenode.net or Puppet Community Slack if you need help w/ any of this. I realize you're not the original person who submitted the change. Let me know if creating the tests will be a problem.

@alexjfisher alexjfisher mentioned this pull request Sep 5, 2017
12 tasks
@wyardley wyardley added the bug Something isn't working label Sep 5, 2017
wyardley pushed a commit to wyardley/puppet-rabbitmq that referenced this pull request Sep 6, 2017
@costela costela closed this Sep 6, 2017
@costela
Copy link
Contributor Author

costela commented Sep 6, 2017

FYI: I've moved this PR to a different branch, and github doesn't seem to support updating the source branch (just the destination branch). So I'll open a new one shortly, including tests and squashed commits.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working needs-squash needs-tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants