(GH-1493) Validate metadata when running or showing tasks #1542
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nicklewis
force-pushed
the
GH-1493-warn-on-unknown-task-metadata
branch
2 times, most recently
from
d41aeb2
to
b0b5055
Compare
|
CLA signed by all contributors. |
nicklewis
force-pushed
the
GH-1493-warn-on-unknown-task-metadata
branch
4 times, most recently
from
eb54868
to
494b87b
Compare
beechtom
reviewed
Jan 22, 2020
nicklewis
force-pushed
the
GH-1493-warn-on-unknown-task-metadata
branch
3 times, most recently
from
f696b44
to
d4dd57e
Compare
lucywyman
reviewed
Jan 24, 2020
| @@ -83,7 +83,7 @@ def get_validated_task(task_name, params = nil) | |||
| raise Bolt::Error.unknown_task(task_name) unless tasksig | |||
|
|
|||
| Bolt::Task::Run.validate_params(tasksig, params) if params | |||
There was a problem hiding this comment.
Do we still need this line (or function) with these changes?
There was a problem hiding this comment.
Yes, this line validates that the parameters provided are acceptable by the task. It might make sense in the future to combine the logic on Bolt::Task::Run into Bolt::Task so this would be like:
task = Bolt::Task.from_task_signature(tasksig)
task.validate_params(params) if params
taskBut for now I don't think it fits with this PR.
lucywyman
reviewed
Jan 24, 2020
| } | ||
| end | ||
|
|
||
| def validate_metadata |
There was a problem hiding this comment.
I think it'd be useful to have a test to verify that this warns with extra keys present - https://github.com/puppetlabs/bolt/blob/master/spec/integration/parsing_spec.rb seems like a good place for that? The sample::params task already has an extra top-level anything key, if we want to just use that.
|
Changelog needs a rebase :P |
|
Rebased |
nicklewis
force-pushed
the
GH-1493-warn-on-unknown-task-metadata
branch
from
d4dd57e
to
9f01efd
Compare
beechtom
approved these changes
Jan 24, 2020
|
CLA signed by all contributors. |
lucywyman
force-pushed
the
GH-1493-warn-on-unknown-task-metadata
branch
from
b0a4843
to
e1debf1
Compare
The previous use of Struct here didn't have many benefits, as it's basically a proper Class. It was also sloppy about what it accepted as arguments, as the metaprogramming involved made it possible to use both keyword or string keys, and made every parameter effectively optional. This is now written as an ordinary class with a positional initialize method. For the common case where we have a TaskSignature object that we want to turn into a Bolt::Task, there is now a from_task_signature factory method. This simplifies the code and makes it easier to keep track of data.
Previously, the get_task_info method used by `bolt task show` expected to receive a task hash generated from a TaskSignature object. As we now have a Bolt::Task class, it makes sense to use that as the standard interface. This also ensures that whether we're inspecting or running a task, we always have a Bolt::Task object to work with.
We now validate metadata during the construction of a Bolt::Task object, which means a warning will be logged when running or showing a task if that task contains metadata fields that are unknown to Bolt. In some cases, an unknown metadata field may signal a typo or that certain keys (such as parameter specifications) are in the wrong place in the file. In others, it may mean that new features have been added to the task spec that the current version of Bolt doesn't understand and may not do the right thing with. In both cases, it's helpful to alert the user to a potential problem.
This adds an integration test to ensure a warning is raised when metadata for a task is invalid.
lucywyman
force-pushed
the
GH-1493-warn-on-unknown-task-metadata
branch
from
e1debf1
to
3c43652
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We now validate metadata during the construction of a Bolt::Task object,
which means a warning will be logged when running or showing a task if that
task contains metadata fields that are unknown to Bolt.
In some cases, an unknown metadata field may signal a typo or that certain
keys (such as parameter specifications) are in the wrong place in the file.
In others, it may mean that new features have been added to the task spec
that the current version of Bolt doesn't understand and may not do the
right thing with. In both cases, it's helpful to alert the user to a
potential problem.
This also includes a refactor of the Bolt::Task class as well as a change to
make
bolt task showuse an instance of the Bolt::Task class (which allowsthis warning to appear in that case).
Closes #1493