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

Minor fixes to validation of runner parameter override #2363

Merged
merged 3 commits into from Jan 8, 2016

Conversation

m4dcoder
Copy link
Contributor

@m4dcoder m4dcoder commented Jan 8, 2016

No description provided.

The type for the headers parameter in the http-request runner should be object. Override of the type attribute is not allowed for runner parameters.
Include the action reference (i.e. core.local) in the error message on action parameter validation. If not included and there are failures from multiple actions on register, it will be difficult to troubleshoot.
Refactor the logic for validating runner parameter overrides to a utility method.
m4dcoder added a commit that referenced this pull request Jan 8, 2016
Minor fixes to validation of runner parameter override
@m4dcoder m4dcoder merged commit fe49eb0 into master Jan 8, 2016
raise InvalidActionParameterException(
'The attribute "%s" for the runner parameter "%s" cannot'
'be overridden.' % (attribute, name))
validate_runner_parameter_attribute_override(
Copy link
Member

Choose a reason for hiding this comment

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

In the future please prefer to use keyword argument when calling functions.

While working on resolving merge conflicts in my PR, I believe I encounter a minor issue - function order is `runner_param_attr_value, action_param_attr_value), but the values here are passed to the function in the reverse order.

Luckily, in this case the order doesn't matter, because those values are only used in a simple != if condition, but it could matter in other scenarios.

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

Successfully merging this pull request may close these issues.

None yet

2 participants