Skip to content

(maint) Reset "tasks" setting after with_script_compiler #7822

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

nicklewis
Copy link
Contributor

The Puppet::Pal.with_script_compiler method sets the
Puppet[:tasks] setting to true for the duration of the block, but
never sets it back to its original state. This means that the mode of
operation the process is in will be globally changed depending on
whether this method has been called. The corresponding
with_catalog_compiler method has similar behavior in setting
Puppet[:tasks] to false, but it ensures that the setting is reset to
its previous value when the method returns. We now copy that behavior in
with_script_compiler.

@puppetcla
Copy link

CLA signed by all contributors.

@shrug shrug requested a review from pcarlisle November 11, 2019 17:54
@shrug
Copy link
Contributor

shrug commented Nov 11, 2019

@pcarlisle Are there any ramifications w/r/t thread-safety with this?

@justinstoller
Copy link
Member

This #7746 & this #7745 represent two different approaches for making this setting threadsafe, this PR doesn't harm either approach (though it would cause a rebase conflict with one it isn't difficult to figure out the correct behavior).

This is a correctness thing and, iirc, parallels what Maggie did to with_catalog_compiler in #7599 (when we began calling with_catalog_compiler in Server to do apply block compilation in plans).

@shrug
Copy link
Contributor

shrug commented Nov 18, 2019

Not sure what's up with that test failure, something weird with that. @nicklewis does someone from the bolt team want to review and merge?

@jtappa jtappa closed this Nov 25, 2019
@jtappa jtappa reopened this Nov 25, 2019
@jtappa
Copy link
Contributor

jtappa commented Nov 25, 2019

close/reopen to kick travis

@jtappa
Copy link
Contributor

jtappa commented Dec 2, 2019

@nicklewis looks like you might need to just close and open a new PR entirely, Travis is still confused :(

The `Puppet::Pal.with_script_compiler` method sets the
`Puppet[:tasks]` setting to true for the duration of the block, but
never sets it back to its original state. This means that the mode of
operation the process is in will be globally changed depending on
whether this method has been called. The corresponding
`with_catalog_compiler` method has similar behavior in setting
`Puppet[:tasks]` to false, but it ensures that the setting is reset to
its previous value when the method returns. We now copy that behavior in
`with_script_compiler`.
@nicklewis nicklewis force-pushed the reset-tasks-setting-in-script-compiler branch from ab21af9 to c2e2aaf Compare December 2, 2019 22:21
@nicklewis
Copy link
Contributor Author

Travis is green now.

@joshcooper joshcooper merged commit 4cae5c9 into puppetlabs:master Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants