-
Notifications
You must be signed in to change notification settings - Fork 105
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
(PDK-1615) Add validator for environment.conf #866
(PDK-1615) Add validator for environment.conf #866
Conversation
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.
Added a few minor comments/suggestions.
As for the grouping, I would lean towards a distinct controlrepo
(or similarly named) group for control-repo meta validations.
env_conf = PDK::ControlRepo.environment_conf_as_config(target) | ||
|
||
env_conf.resolve.each do |setting_name, setting_value| | ||
setting_name = setting_name.slice(12..-1) |
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.
I'm guessing this is slicing off an environment.
prefix? Adding a comment about that or even using gsub
(performance implications should be pretty minor) would help clarify this for future us.
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.
Added a comment.
end | ||
|
||
context 'when a target is provided that has all allowed settings and is valid' do | ||
let(:target) { { name: 'environment.conf', content: "modulepath=foo\nmanifest=foo\nconfig_version=foo\nenvironment_timeout=0" } } |
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.
Maybe move content
to it's own var/let
block and use a heredoc to make it easier to maintain?
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.
I managed to use a heredoc inside the hash which looks ok e.g.
let(:target) do
{
name: 'environment.conf',
content: <<-EOT
modulepath=foo
manifest=foo
config_version=foo
environment_timeout=0
EOT
}
end
spec/unit/pdk/validate/puppet/puppet_environment_conf_validator_spec.rb
Outdated
Show resolved
Hide resolved
I think this should be in a control repo validator group |
44ca412
to
1aa7d21
Compare
This commit adds a validator for the environment.conf file when being used inside a control repository. The validator complies with the rules set out in https://puppet.com/docs/puppet/latest/config_file_environment.html. This commit also adds tests for the validator setup, and various example file content and expected validation results.
1aa7d21
to
7ffa17f
Compare
Moved it to it's own validator group e.g.
and
|
This commit cleans up the autoloading statements to be an alphabetical order.
This commit adds a validator for the environment.conf file when being used
inside a control repository. The validator complies with the rules set out in
https://puppet.com/docs/puppet/latest/config_file_environment.html.
This commit also adds tests for the validator setup, and various example file
content and expected validation results.
TODO
puppet
validator group OR should it be in a separate validator group e.g.controlrepo
?