Skip to content
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

Add validation to transition type #5

Merged
merged 1 commit into from
Mar 16, 2016

Conversation

elyscape
Copy link
Contributor

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.

@elyscape
Copy link
Contributor Author

Any update on this?

@elyscape
Copy link
Contributor Author

Pinging @hunner, @reidmv.

@reidmv
Copy link
Contributor

reidmv commented Mar 16, 2016

@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.

end
res = @resource[:resource]
value.keys.each do |attribute|
unless res.valid_parameter?(attribute)
Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found the bug.

@elyscape
Copy link
Contributor Author

@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?

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.
@elyscape
Copy link
Contributor Author

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.

@reidmv
Copy link
Contributor

reidmv commented Mar 16, 2016

@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.

reidmv added a commit that referenced this pull request Mar 16, 2016
Add validation to transition type
@reidmv reidmv merged commit 13fbe99 into puppetlabs:master Mar 16, 2016
@reidmv
Copy link
Contributor

reidmv commented Mar 16, 2016

@hunner what are next steps for cutting a Forge release, given that transition is in the puppetlabs namespace?

@elyscape elyscape deleted the add_validation branch March 16, 2016 20:39
cegeka-jenkins pushed a commit to cegeka/puppet-transition that referenced this pull request Sep 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants