-
Notifications
You must be signed in to change notification settings - Fork 21
Add validation to transition type #5
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
Any update on this?
@elyscape whoa, thanks for doing this! This didn't get on my radar. Sorry for the radio silence on the PR.
I really like the general form of this. It looks like there are two problems that need to be resolved before it can be merged. I'll comment inline on where I'm having problems running the new code.
lib/puppet/type/transition.rb
Outdated
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.
Running under Puppet 4.3.2.
Manifest:
file { '/tmp/test': ensure => file, content => 'enabled=1', before => Notify['test'], } notify { 'test': } transition { 'stop myapp service': resource => File['/tmp/test'], attributes => { ensure => absent }, prior_to => Notify['test'] }
Error:
[reidmv@halcyon:~/src/puppetlabs-transition/] % puppet apply test2.pp
Notice: Compiled catalog for halcyon.corp.puppetlabs.net in environment production in 0.05 seconds
Error: Parameter attributes failed on Transition[stop myapp service]: Validate method failed for class attributes: undefined method `valid_parameter?' for #<Puppet::Type::File:0x007faca4851788> at /Users/reidmv/src/puppetlabs-transition/test2.pp:9
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.
Found the bug.
a6264ed to
835f86b
Compare
@reidmv Since this pull request is reworking the validation, should I also remove the type-wide validate method, which only ensures that resource, attributes, and prior_to are provided, and just put isrequired on those attributes? Is there a reason you went that route instead of using isrequired?
667122e to
c6c6efc
Compare
This adds validations to ensure that: 1. The value of `resource` is either a valid resource reference or a string that can be converted to a valid resource reference. 2. The resource specified in `resource` exists in the catalog. 3. The value of `attributes` is a hash. 4. All the specified attributes are valid parameters for the specified resource. 5. The values of `prior_to` are valid resource references or strings that can be converted to valid resource references. 6. All resources specified in `prior_to` exist in the catalog. Additionally, it adds munging to resolve the values of `resource` and `prior_to` into their catalog representations and removes the now-redundant resource resolution from the provider.
89fd940 to
e4bfe35
Compare
Okay, validation and munging moved to the pre_run_check method so that we can guarantee that resources will be in the catalog if they actually exist. Please weigh in on whether I should add isrequired to the resource, attributes, and prior_to attributes.
@elyscape Thank you, this looks great! I'm 👍 to merge.
Regarding isrequired, I don't remember why I chose to do a global validate. I don't think it was functional, it would be fine to switch to just specifying isrequired on those properties/parameters. If you'd like to open a second PR on that I'll merge it.
Add validation to transition type
@hunner what are next steps for cutting a Forge release, given that transition is in the puppetlabs namespace?
Add validation to transition type
This adds validations to ensure that:
resourceis either a valid resource reference or a string that can be converted to a valid resource reference.resourceexists in the catalog.attributesis a hash.prior_toare valid resource references or strings that can be converted to valid resource references.prior_toexist in the catalog.Additionally, it adds munging to resolve the values of
resourceandprior_tointo their catalog representations and removes the now-redundant resource resolution from the provider.