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.

Sign up for GitHub

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

Jump to bottom

Network Routers REST API #15450

Merged
merged 2 commits into from
Jul 24, 2017
Merged

Network Routers REST API #15450

merged 2 commits into from
Jul 24, 2017

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Jun 27, 2017

Add capability to retrieve network routers directly

@gildub gildub changed the title Network Routers REST API [WIP] Network Routers REST API Jun 27, 2017
@miq-bot miq-bot added the wip label Jun 27, 2017
@gildub gildub changed the title [WIP] Network Routers REST API Network Routers REST API Jul 7, 2017
@miq-bot miq-bot removed the wip label Jul 7, 2017
@imtayadeway
Copy link
Contributor

@gildub this looks promising, but can you add some tests to prove that this works?

@gildub
Copy link
Contributor Author

gildub commented Jul 12, 2017

@imtayadeway, I added the test file, thanks.

imtayadeway
imtayadeway suggested changes Jul 12, 2017
View reviewed changes
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.

Looks great! I just have the exact same questions as #15524

@gildub
Copy link
Contributor Author

gildub commented Jul 14, 2017

@imtayadeway,

  • I've removed the subcollection
  • I've added tests for the bulk query in spec/requests/api/collections_spec.rb for the query itself (200) and in spec/requests/api/network_routers_spec.rb for the (403)
  • I've also added the default query in spec/requests/api/collections_spec.rb which I believe makes parts of spec/requests/api/network_routers_spec.rb redundant

imtayadeway
imtayadeway reviewed Jul 14, 2017
View reviewed changes
spec/requests/api/collections_spec.rb Outdated
@@ -134,6 +134,11 @@ def test_collection_bulk_query(collection, collection_url, klass, id = nil)
test_collection_query(:hosts, hosts_url, Host, :guid)
end

it 'queries NetworkRouters' do
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar question to https://github.com/ManageIQ/manageiq/pull/15524/files#r127473371

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@imtayadeway, sure although I believe all those test could go in the collection but that would be the purpose of another PR ;)

@miq-bot
Copy link
Member

miq-bot commented Jul 19, 2017

Checked commits https://github.com/gildub/manageiq/compare/df825c7fc26f429ba8209e18a03a0670f44ef1a7~...cde93c57c21bfd3cd77d3673d9607bdd896e033e with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

imtayadeway
imtayadeway approved these changes Jul 19, 2017
View reviewed changes
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.

Great work @gildub , thanks!

@abellotti
Copy link
Member

Thanks @gildub for the API Enhancement. LGTM !!

@abellotti abellotti merged commit 1e43457 into ManageIQ:master Jul 24, 2017
@abellotti abellotti added this to the Sprint 65 Ending Jul 24, 2017 milestone Jul 24, 2017
@gildub gildub deleted the network_router-API-initial branch June 27, 2018 01:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imtayadeway imtayadeway imtayadeway approved these changes

@jntullo jntullo Awaiting requested review from jntullo

Assignees

@abellotti abellotti

Labels
api enhancement providers/network
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@gildub @imtayadeway @miq-bot @abellotti @chessbyte