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 cloud subnet REST API #15248

Merged
merged 1 commit into from
Jun 2, 2017
Merged

Add cloud subnet REST API #15248

merged 1 commit into from
Jun 2, 2017

Conversation

gildub
Copy link
Contributor

@gildub gildub commented May 30, 2017

Need to be able to retrieve cloud subnets resources directly for ManageIQ/manageiq-providers-openstack#36

Depends upon ManageIQ/manageiq-ui-classic#1508

@gildub
Copy link
Contributor Author

gildub commented May 30, 2017

Skipping test [1] because it fails with undefined method `[]' for nil:NilClass

     # ./spec/support/api_helper.rb:112:in `action_identifier'
     # ./spec/support/api_helper.rb:117:in `collection_action_identifier'
     # ./spec/requests/api/collections_spec.rb:25:in `test_collection_bulk_query'
     # ./spec/requests/api/collections_spec.rb:570:in `block (3 levels) in <top (required)>'

[1] https://github.com/ManageIQ/manageiq/pull/15248/files#diff-a379444a5c62b1302da96d4a02592b6eR566

@gildub
Copy link
Contributor Author

gildub commented May 30, 2017

@jntullo, could you please help with the failing test?

it "bulk query CloudSubnets" do
skip "collection_action_identifier gets undefined method `[]' for nil:NilClass"
FactoryGirl.create(:cloud_subnet)
test_collection_bulk_query(:could_subnets, cloud_subnets_url, CloudSubnet)
Copy link
Contributor

@imtayadeway imtayadeway May 30, 2017

Choose a reason for hiding this comment

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

This test is failing because of a typo - s/could/cloud/

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, thanks! 👍

@gildub
Copy link
Contributor Author

gildub commented May 31, 2017

@jntullo, don't worry about it :).

@miq-bot
Copy link
Member

miq-bot commented May 31, 2017

Checked commit https://github.com/gildub/manageiq/commit/d63393e98cfbc0ed4e58ac1ad14836d10b2f5579 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 0 offenses detected
Everything looks fine. ⭐

@abellotti
Copy link
Member

@gildub thanks for the enhancement, this LGTM!!

@imtayadeway can you give a 👍 or approve if ok by you ? Thanks.

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.

LGTM :shipit:

Thanks @gildub !

@abellotti abellotti added this to the Sprint 62 Ending Jun 5, 2017 milestone Jun 2, 2017
@abellotti abellotti merged commit 50ead8d into ManageIQ:master Jun 2, 2017
@gildub
Copy link
Contributor Author

gildub commented Jun 5, 2017

@imtayadeway, thank you too!

@simaishi
Copy link
Contributor

simaishi commented Nov 1, 2017

@gildub Is this PR supposed to be fine/yes? The label is there, but I can't see where the bot request to add bug and fine/yes came from...

@gildub
Copy link
Contributor Author

gildub commented Nov 1, 2017

@simaishi, that's me, sorry I removed the miq-bot comment I used, which makes me realise how helpful it could be to trace back things, therefore I won't do it again.

Anyway this is also needed by https://bugzilla.redhat.com/show_bug.cgi?id=1467725.

@simaishi
Copy link
Contributor

simaishi commented Nov 1, 2017

@gildub ha ha.. thanks! I'm glad the source is identified 😄

So... this PR depends on ManageIQ/manageiq-ui-classic#1508, but it's currently marked as fine/no. That can be fine/yes?

@gildub
Copy link
Contributor Author

gildub commented Nov 2, 2017

@simaishi, I don't see any restrictions for ManageIQ/manageiq-ui-classic#1508 to go to Fine (just replaced the fine label). Other similar PRs have made to Fine, the dependent pieces (new buttons, etc) have already been ported back.

@simaishi
Copy link
Contributor

simaishi commented Nov 2, 2017

Backported to Fine via #16150

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.

6 participants