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 guest_devices support to the API #7

Merged
merged 3 commits into from
Sep 7, 2017

Conversation

skovic
Copy link

@skovic skovic commented Aug 8, 2017

This PR adds guest_devices support to the API.

@skovic
Copy link
Author

skovic commented Aug 8, 2017

@miq-bot add_label wip

@miq-bot miq-bot changed the title Add guest_devices support to the API [WIP] Add guest_devices support to the API Aug 9, 2017
@miq-bot miq-bot added the wip label Aug 9, 2017
@skovic
Copy link
Author

skovic commented Aug 25, 2017

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Add guest_devices support to the API Add guest_devices support to the API Aug 25, 2017
@miq-bot miq-bot removed the wip label Aug 25, 2017
@skovic
Copy link
Author

skovic commented Aug 30, 2017

@abellotti Can we get this PR merged now that ManageIQ/manageiq#15371 has been merged? Thanks

@skovic skovic closed this Aug 30, 2017
@skovic skovic reopened this Aug 30, 2017
@abellotti
Copy link
Member

This LGTM!!. @imtayadeway quick 👀 when you get a chance ? Thanks.

end
end

context "with an invalid role" do
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but wording here is misleading. It's not that the role is invalid, it's that the user's role does not authorize them to use this feature. Similarly, it doesn't fail - it does the right thing! i.e. respond with a forbidden status. Let's merge this as-is though, maybe you could consider rewording this in a follow up?

Copy link
Contributor

@imtayadeway imtayadeway left a comment

Choose a reason for hiding this comment

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

👍

@miq-bot
Copy link
Member

miq-bot commented Aug 31, 2017

Checked commits skovic/manageiq-api@540d142~...25b7d1d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🍰

@skovic
Copy link
Author

skovic commented Aug 31, 2017

@imtayadeway I reworded the descriptions in the test file. Let me know if it looks OK now. Also, I noticed that guest_devices_spec.rb was in spec/requests/api/ instead of spec/requests/ so I moved it to spec/requests/ -- this must have happened while migrating these changes to this repo.

@imtayadeway
Copy link
Contributor

@skovic thanks for making those changes! 😻 This looks great - it's just failing because of some upstream breakages - I'll restart your build once those are under control.

@juliancheal
Copy link
Member

@imtayadeway yeah still looks like it's failing, what's the upstream issue causing it?

@rodneyhbrown7
Copy link

@juliancheal , @blomquisg

@abellotti
Copy link
Member

Thanks @skovic for the API Enhancement !! 👍

@abellotti abellotti merged commit 4a08be7 into ManageIQ:master Sep 7, 2017
@abellotti abellotti added this to the Sprint 69 Ending Sep 18, 2017 milestone Sep 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants