-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Always store the path in the label_to_path_map #1883
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
Always store the path in the label_to_path_map #1883
Conversation
Otherwise the path won't be found next time the provider is asked for it
|
Waiting for CLA signature by @tbartelmess @tbartelmess - We require a Contributor License Agreement (CLA) for people who contribute to Puppet, but we have an easy click-through license with instructions, which is available at https://cla.puppetlabs.com/ Note: if your contribution is trivial and you think it may be exempt from the CLA, please post a short reply to this comment with details. http://docs.puppetlabs.com/community/trivial_patch_exemption.html |
|
CLA signed by all contributors. |
|
Thank you very much for this contribution! Since this corrects a bug in existing behavior it should have tests to ensure that this behavior doesn't regress. Would you be willing to add a test case to make sure this the path is properly stored? For reference, the existing specs are at https://github.com/puppetlabs/puppet/blob/master/spec/unit/provider/service/launchd_spec.rb and could be used as a reference for adding the new test. |
|
Also, if you would like any help with the tests I would be happy to lend a hand. If you would like to discuss this further, IRC would be one of the best ways to get ahold of me. I'm generally available on irc.freenode.net from 9:00 AM - 5:00 PM in #puppet and #puppet-dev, and my nickname is finch. |
|
@adrienthebo I've added a unit test. However, I've noticed an other issue with the launchd provider in the jobsearch function: When the provider once read the jobs, new jobs are ignored. I think it would make sense to re-read all plists in the case that the job was not found. Should I open a new ticket/pull-request or is it fine when I add it to this pull request |
|
Do you think that fixing the other issue would be a significant change, or only a minor revision to the existing code? If it's a non-trivial fix it would be best to open a new pull request for that. |
|
It's fairly trivial. See https://github.com/tbartelmess/puppet/compare/puppetlabs:master...d6033e4 |
|
While the change does just extract existing behavior, it is a fairly big refactor. I think it would be best to get this change in and then work on the bug/refactor in a new pull request. Before I merge this, could you squash the commits in this pull request down to a single commit and amend the commit to match our project standards? Housekeeping - Commit MessageIt would be good to have the commit message amended a little. The first line of the commit should have the issue number and a description of the problem or fix, and the body of the commit message should explain what is this existing behavior and how this change fixes it. We have a more complete explanation of this at https://github.com/puppetlabs/puppet/blob/master/CONTRIBUTING.md#making-changes . |
|
summary: fixed up and merged into master in 15e81ed; this should be released in 3.4.0. Thanks for the contribution! |
|
gh-1914 is the refactor you're discussing; in fact, it supersedes this pull req. |
Otherwise the path won't be found next time the provider is asks for it.
https://projects.puppetlabs.com/issues/22458