-
Notifications
You must be signed in to change notification settings - Fork 23
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
Fixes #24, #25 - validation of optional parameters containing required attributes #26
Conversation
It would be great if @komidore64, @tstrachota or @thomasmckay could review/test this PR. |
7c42f4a
to
e8a8d16
Compare
cool deal! ack 🍺 |
BATS are green with this patch - http://ci.theforeman.org/job/systest_foreman_hammer_nightly/111/ |
Even the not fake BATs are green, @tstrachota: http://ci.theforeman.org/job/systest_foreman_hammer_nightly/112/ |
tree | ||
end | ||
def validate!(parameters) | ||
errors = validate(params, parameters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about using values
instead of parameters
. the params
vs parameters
might be a bit misleading
Nice job overall. Some comments inline |
ACK |
Response object has changed parameters in constructor
affdb7d
to
5092c89
Compare
@iNecas could you please take a look at my new validation attempt (in separate commit that will eventualy get squashed)? |
Small suggestions, leaving up to you if incorporating them or not. ACK |
278bf7a
to
c1bd32c
Compare
Thanks @iNecas! I'll rerun BATs and merge if green. |
This PR should fix #24 and #25.
As a bonus I've added extra commit allowing to disable validation per call.