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

Prevent unsupported classes from being added #18842

Merged
merged 1 commit into from
Jun 27, 2019
Merged

Prevent unsupported classes from being added #18842

merged 1 commit into from
Jun 27, 2019

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jun 6, 2019 •
edited
Loading

https://bugzilla.redhat.com/show_bug.cgi?id=1671844

Unsupported providers shouldn't be able to be added.

It's marked as enhancement cause the attached RFE, but it addresses a more general bug. So really it ought to be a bug.

@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 6, 2019

@miq-bot assign @jrafanie
@miq-bot add_label enhancement

@Fryguy
Copy link
Member

Fryguy commented Jun 7, 2019

@agrare Please review.

@Fryguy Fryguy requested a review from agrare June 7, 2019 20:12
@d-m-u d-m-u force-pushed the adding_supported_for_create_check_to_ems branch from 75374ae to ef30bdb Compare June 10, 2019 15:43
@d-m-u d-m-u changed the title [WIP] Prevent unsupported classes from being added Prevent unsupported classes from being added Jun 10, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 10, 2019

@miq-bot remove_label wip

@miq-bot miq-bot removed the wip label Jun 10, 2019
@d-m-u d-m-u force-pushed the adding_supported_for_create_check_to_ems branch 2 times, most recently from 118824c to 1581d20 Compare June 10, 2019 15:45
jrafanie
jrafanie approved these changes Jun 10, 2019
View reviewed changes
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM, merge when green

@bdunne
Copy link
Member

bdunne commented Jun 10, 2019

It seems that the before_create check is no longer in this PR, so it doesn't prevent unsupported classes from being added.

irb(main):001:0> ManageIQ::Providers::Google::CloudManager.supported_for_create?
=> false
irb(main):002:0> ManageIQ::Providers::Google::CloudManager.create!(:hostname => "abc", :name => "abc", :zone => Zone.find_by(:name => "default"))
=> #<ManageIQ::Providers::Google::CloudManager id: 7, name: "abc", created_on: "2019-06-10 17:21:04", updated_on: "2019-06-10 17:21:04", guid: "b85a8429-aa0c-41b3-92a6-c0866b899382", zone_id: 2, type: "ManageIQ::Providers::Google::CloudManager", api_version: nil, uid_ems: nil, host_default_vnc_port_start: nil, host_default_vnc_port_end: nil, provider_region: nil, last_refresh_error: nil, last_refresh_date: nil, provider_id: nil, realm: nil, tenant_id: 1, project: nil, parent_ems_id: nil, subscription: nil, last_metrics_error: nil, last_metrics_update_date: nil, last_metrics_success_date: nil, tenant_mapping_enabled: nil, enabled: true, options: nil, zone_before_pause_id: nil, last_inventory_date: nil>

@d-m-u d-m-u force-pushed the adding_supported_for_create_check_to_ems branch 7 times, most recently from 1e0b28c to 2652285 Compare June 12, 2019 11:02
@d-m-u d-m-u changed the title Prevent unsupported classes from being added [WIP] Prevent unsupported classes from being added Jun 12, 2019
@miq-bot miq-bot added the wip label Jun 12, 2019
@d-m-u d-m-u force-pushed the adding_supported_for_create_check_to_ems branch 5 times, most recently from dd80db2 to 2dc355b Compare June 12, 2019 13:02
@d-m-u d-m-u changed the title [WIP] Prevent unsupported classes from being added Prevent unsupported classes from being added Jun 25, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 25, 2019

@miq-bot remove_label wip
anyone want to review please please please?

@miq-bot miq-bot removed the wip label Jun 25, 2019
@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 26, 2019

@jrafanie @bdunne please review

jrafanie
jrafanie reviewed Jun 26, 2019
View reviewed changes
spec/models/manageiq/providers/inventory/persister/targeted_refresh_spec_helper.rb
@@ -7,7 +7,7 @@ def create_persister
end

def expected_ext_management_systems_count
2
3
Copy link
Member

Choose a reason for hiding this comment

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

haha, so obvious

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well it's the same as https://github.com/ManageIQ/manageiq/pull/18842/files#r293334487 so yes.

Copy link
Member

Choose a reason for hiding this comment

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

yes, I was being sarcastic... it was an existing test... I'm being silly

jrafanie
jrafanie approved these changes Jun 26, 2019
View reviewed changes
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM if @bdunne question is resolved

@d-m-u d-m-u force-pushed the adding_supported_for_create_check_to_ems branch from 2c4f168 to 6aef887 Compare June 27, 2019 14:43
@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 27, 2019

@bdunne @jrafanie can someone please put this out of its misery already?

@d-m-u
Copy link
Contributor Author

d-m-u commented Jun 27, 2019

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Jun 27, 2019
jrafanie
jrafanie approved these changes Jun 27, 2019
View reviewed changes
Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

LGTM, feel free to merge @bdunne

bdunne
bdunne approved these changes Jun 27, 2019
View reviewed changes
Copy link
Member

@bdunne bdunne left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@bdunne bdunne merged commit 84b8a0a into ManageIQ:master Jun 27, 2019
@bdunne bdunne added this to the Sprint 115 Ending Jul 8, 2019 milestone Jun 27, 2019
@bdunne bdunne assigned bdunne and unassigned jrafanie Jun 27, 2019
@bdunne bdunne added the test label Jun 27, 2019
This was referenced Jun 27, 2019
Merged
Merged
Merged
@himdel himdel mentioned this pull request Jun 28, 2019
Closed
This was referenced Jun 28, 2019
Merged
Merged
@aufi aufi mentioned this pull request Jul 2, 2019
Merged
@d-m-u d-m-u deleted the adding_supported_for_create_check_to_ems branch September 26, 2019 10:37
@jrafanie jrafanie mentioned this pull request Sep 27, 2019
Merged
@d-m-u d-m-u mentioned this pull request Sep 27, 2019
Merged
@jrafanie jrafanie mentioned this pull request Sep 30, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fryguy Fryguy Fryguy left review comments

@bdunne bdunne bdunne approved these changes

@jrafanie jrafanie jrafanie approved these changes

@agrare agrare Awaiting requested review from agrare agrare is a code owner

Assignees

@bdunne bdunne

Labels
bug enhancement test
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@d-m-u @Fryguy @bdunne @jrafanie @miq-bot