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 possibility to produce a detailed error message #254

Merged
merged 3 commits into from
Aug 23, 2021

Conversation

reidmv
Copy link
Contributor

@reidmv reidmv commented Aug 19, 2021

Previously, the validation procedures were occuring in the #munge
method. This was a problem because if #munge raises an exception, the
error displayed to the user is opaque. If the data input is not valid,
the error should be raised in the #validate method. This will ensure
that the output given to the user includes information about the class,
resource, and parameter which is in error.

@reidmv reidmv requested a review from a team as a code owner August 19, 2021 18:28
@puppet-community-rangefinder
Copy link

registry_value is a type

Breaking changes to this file WILL impact these 40 modules (exact match):
Breaking changes to this file MAY impact these 24 modules (near match):

This module is declared in 8 of 578 indexed public Puppetfiles.


These results were generated with Rangefinder, a tool that helps predict the downstream impact of breaking changes to elements used in Puppet modules. You can run this on the command line to get a full report.

Exact matches are those that we can positively identify via namespace and the declaring modules' metadata. Non-namespaced items, such as Puppet 3.x functions, will always be reported as near matches only.

Previously, the validation procedures were occuring in the #munge
method. This was a problem because if #munge raises an exception, the
error displayed to the user is opaque. If the data input is not valid,
the error should be raised in the #validate method. This will ensure
that the output given to the user includes information about the class,
resource, and parameter which is in error.
@reidmv reidmv force-pushed the fix-munge-validate branch from afcdb72 to 551fab7 Compare August 19, 2021 18:34
@binford2k
Copy link
Contributor

@reidmv if I'm reading this correctly, it doesn't add or remove any functionality at all, it just moves it to become more visible. Is that correct?

reidmv added 2 commits August 19, 2021 16:46
This property previously set a default value of empty string, or ''.

An empty string is not a valid dword or qword. This commit modifies the
data property to not set a default value when the type is dword or
qword.

We probably shouldn't ever set a default value for data. For now though,
by limiting the change to dword and qword types, we should avoid making
any functional changes.
In order to enforce consistency, since the data property sometimes has a
default value now and sometimes doesn't, make the property required.

When we make the default value consistent, we will have the option of
making this property not required.
@reidmv
Copy link
Contributor Author

reidmv commented Aug 20, 2021

@binford2k that is correct, or, that is the intention, yes. The objective is to eliminate the possibility of an error like the following from being encountered:

Error: Failed to apply catalog: The data must be a valid DWORD:

And instead return an error like this one:

Error: Validation of Registry_value[HKLM\System\Value] failed: No value supplied for required property 'data' (line: 1)

Note that I've just added another two commits. I discovered that the real problem wasn't where the error was raised, munge vs. validate, but that the default value for the property wasn't passing validation when type was set to dword or qword. So, this commit now does include an additional internal change: it removes the invalid default value for those data types, but adds a type-level validation to require that the user set the property.

Unfortunately, it doesn't look like there's currently an easier way to produce a sane error message without changing the behavior of registry_value in some way (even if minor). Puppet doesn't seem to have been built to accommodate the scenario of a type supplying a default value that doesn't pass its own validation routines... 🤷

@daianamezdrea daianamezdrea changed the title (maint) move validation failures to #validate def Add possibility to produce a detailed error message Aug 23, 2021
@daianamezdrea daianamezdrea merged commit 5773f66 into puppetlabs:main Aug 23, 2021
@daianamezdrea
Copy link
Contributor

Thank you for your contribution, @reidmv !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants