-
Notifications
You must be signed in to change notification settings - Fork 2.2k
(bug) - Honour custom insync message in noop mode #9339
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
Conversation
fb91f78
to
2d9d761
Compare
2d9d761
to
21f5b19
Compare
Thanks @jordanbreen28 Is there a particular module we can use to demonstrate the issue? |
hey @joshcooper this actually came through to us as a bug for the resource_api in this issue puppetlabs/puppet-resource_api#307. You can see in this comment that after a bit of playing around I was able to replicate this problem. AFAIK there is no module I can think of that uses a custom insync message, I had to implement my own version of this example in a dsc module I had lying about in one of my environments open. puppetlabs-firewall firewallchain type includes the |
21f5b19
to
aa8ee3a
Compare
Thanks @jordanbreen28 Could you add a test for this case (should be in |
@joshcooper sure! I'll try get some tests added shortly |
aa8ee3a
to
bf9ba0f
Compare
hey @joshcooper, got a unit test added. Hope that suffices! |
bf9ba0f
to
cb92487
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, could you squash your commits?
cb92487
to
201c2ac
Compare
@joshcooper squashed 🙂 rubocop errors seem unrelated.. |
@jordanbreen28 rubocop released a new gem that flagged some buggy code in puppet. That's since been resolved. Can you rebase your PR against the |
Prior to this commit, a custom insync message was not honoured when applying a manifest in noop mode. This was later tracked down the the `noop` method found in the resource harness class. I've updated the method to match that of the `sync` method below, ensuring that the functionality of the custom isync provider feature is not any different whether running puppet in noop or no-noop mode.
201c2ac
to
365c5a6
Compare
@joshcooper done! |
jenkins please test this |
1 similar comment
jenkins please test this |
jenkins please test this on redhat8-64 |
@jordanbreen28 could you push your PR to your fork of puppet? |
@joshcooper done! Closing in favour of #9443 |
Prior to this PR, a custom insync message was not honoured when applying a manifest in
noop
mode. This was later tracked down to the offendingnoop
method found in the resource harness class, which did not call thechange_to_s
method to retrieve the custom insync message.I've updated the method to match that of the
sync
method below, ensuring that the functionality of the custom isync provider feature is not any different whether running puppet in noop or no-noop mode.Can see the two methods here
Additional Context
Have opted out of adding a space between
(noop)
andaudit_message
to match the three other implementations of this in thesync
andnoop
methods.