Add title consistency checks for multi-namevar providers #240
Conversation
|
I took a stab at resolving #219. It doesn't verify titles returned from types with a single namevar, but perhaps it should make sure those match as well? It also doesn't verify that composite namevar types return a title. That requirement is stated in the documentation, but the code seems to gracefully accommodate the lack of a title. |
| @@ -5,6 +5,16 @@ | |||
| docs: <<-EOS, | |||
| This type provides Puppet with the capabilities to manage ... | |||
| EOS | |||
| title_patterns: [ | |||
| { | |||
There was a problem hiding this comment.
Please add a title_patterns test type instead of changing this one, to preserve multiple-namevars-no-title_patterns testing.
There was a problem hiding this comment.
This changes in this file are no longer necessary after improving the title verification logic and I have removed it.
There was a problem hiding this comment.
Actually, I take back that last comment. I think I need to add a title_patterns definition because that acceptance test deals with multiple namevars and has strict enabled, which means this patch has it trigger errors. Is your expectation that a type with multiple namevars and no title_patterns should emit no warnings or errors?
There was a problem hiding this comment.
Yes, my expectation is that a type with multiple namevars and no title_patterns should emit no warnings or errors. Resources with multiple namevars can be specified by passing in values as properties rather than through the title. While a valid design choice in some rare edge-cases, I'm not overly fond of it. Let's keep this acceptance test as it is, but please add a unit test (and associated code) to make sure that title_patterns are not required.
There was a problem hiding this comment.
I've added the relaxation of the check in a separate commit. If you're fine with it, we can get this merged.
There was a problem hiding this comment.
Yup, looks great! Thanks for doing that. It would have been at least a few days before I could have gotten to it.
seanmil
force-pushed
the
issue_219-title_consistency_check
branch
3 times, most recently
from
ff4d14d to
4b078be
Compare
| @@ -5,6 +5,16 @@ | |||
| docs: <<-EOS, | |||
| This type provides Puppet with the capabilities to manage ... | |||
| EOS | |||
| title_patterns: [ | |||
| { | |||
There was a problem hiding this comment.
Yes, my expectation is that a type with multiple namevars and no title_patterns should emit no warnings or errors. Resources with multiple namevars can be specified by passing in values as properties rather than through the title. While a valid design choice in some rare edge-cases, I'm not overly fond of it. Let's keep this acceptance test as it is, but please add a unit test (and associated code) to make sure that title_patterns are not required.
| @@ -5,6 +5,16 @@ | |||
| docs: <<-EOS, | |||
| This type provides Puppet with the capabilities to manage ... | |||
| EOS | |||
| title_patterns: [ | |||
| { | |||
There was a problem hiding this comment.
I've added the relaxation of the check in a separate commit. If you're fine with it, we can get this merged.
This ensures results returned from a multi-namevar provider meet the following consistency checks: * There must be a title attribute returned * The title value must match one of the title patterns * The values parsed from the title pattern match must match the values returned from the provider for the respective namevars.
Resources with multiple namevars can be specified by passing in values as properties rather than through the title. A valid design choice in some rare edge-cases. This commit allows it through the title-validation.
DavidS
force-pushed
the
issue_219-title_consistency_check
branch
from
a3ca856 to
7f8e987
Compare
|
Rebased on top of master. Will merge after CI confirms git didn't mess up. |
This ensures that title values, if returned from a multi-namevar
provider, meet the following consistency checks:
values returned from the provider for the respective namevars.