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
(PUP-3239) Stop compilation if $environment has bad manifest settings #3117
(PUP-3239) Stop compilation if $environment has bad manifest settings #3117
Conversation
This looks good @demophoon, but we don't have coverage that $environment inside environmentpath or basemodulepath will halt compilation. How about a spec/integration/environments test with a couple of cases similar to the default_manifest_specs 'refuses to compile' spec? |
CLA signed by all contributors. |
@jpartlow I've added the integration tests |
b3bbe5f
to
5c6d0f4
Compare
|
||
# Validates a manifest for this environment. If the manifest cannot be | ||
# validated for the environment then an error is raised. The possible errors | ||
# that can be raised are |
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.
You left this hanging :) And I think we can just say that "If the manifest cannot be validated for the environment, then a Puppet::Error is raised."
5c6d0f4
to
5a55c52
Compare
# in either of the settings then return true, otherwise return false. | ||
# @api public | ||
def unsafe_manifest_interpolation? | ||
return /\$environment/ =~ Puppet.settings.value(:environmentpath, name, true) || /\$environment/ =~ Puppet.settings.value(:basemodulepath, name, true) |
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.
Let's break this line
Other than that and the (presumably) spec order issue on 1.8.7, the one thing we still don't have are specs for unsafe_manifest_interpolation? in spec/unit/node/environment_spec.rb |
f480044
to
f69d984
Compare
@jpartlow I've revised that function to return booleans, i've also added spec tests for it :) |
Not sure why those tests are failing in spec/unit/provider/service/systemd_spec.rb on Ruby 1.8.7 after adding spec tests.. |
@demophoon the 1.8.7 systemd_spec failures were unrelated. I just pushed a fix for those. |
@kylog Sweet, Thanks! Should I rebase so that I have those changes as well? |
@demophoon I just restarted the build. Pretty sure you don't need to rebase explicitly; github does some git magic so you build against latest + your-PR. |
@@ -104,6 +104,21 @@ def resource(type, title) | |||
expect { Puppet::Parser::Compiler.compile(@node) }.to raise_error(Puppet::Error, /disable_per_environment_manifest.*true.*environment.conf.*manifest.*conflict/) | |||
end | |||
|
|||
it "fails if $environment is in environmentpath or basemodulepath" do | |||
conflicted_environment = Puppet::Node::Environment.create(:testing, [], '/some/environment.conf/manifest.pp') |
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.
The path in the test here is a little misleading. The environment.conf
is a file, not a directory. The path would likely be more like '/some/environment/testing/manifests/manifest.pp'
.
Prior to this, the compiler was responsible for testing the environment and producing an error message. This change pulls the validation step into node environment, breaking the error message up so that any component can check if an environment has setting conflicts and return a uniform error message. This will make it easier to add other environment checks. We refrained from including a more general error message handling mechanism similar to what is currently in POPS because generalizing that is a bigger task more suited to a broader internationalization epic.
776a2a2
to
e118d22
Compare
…onment-validation (PUP-3239) Stop compilation if $environment has bad manifest settings
Before this commit we raised a warning if '$environment' was found
in the 'environmentpath' or 'basemodulepath' settings but continued
catalog compilation.
This commit adds in a validation method which raises an error before
compilation if an unsafe interpolation of '$environment' was found in
the 'environmentpath' or 'basemodulepath' settings thus stopping the
compilation process all together. This commit also consolidates the
'conflicting_manifest_settings?' method into the 'validate' method since
it also needs to halt compilation.
When new pre-compilation validations are added they can be added to the
new 'validate' method.