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

[WIP] Fixes logic for allowed_cloud_networks #16804

Closed
wants to merge 1 commit into from

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Jan 11, 2018

This fixes an issue discovered late yesterday wherein the filtering done on cloud network lists was returning incorrect values as of the merge of #16688

WIP because @syncrou and I are conversating about what to do with this

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

@miq-bot assign @gmcculloug
@syncrou can you review please?

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

@miq-bot add_label provisioning

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

@miq-bot add_label gaprindashvili/yes

@miq-bot
Copy link
Member

miq-bot commented Jan 11, 2018

Checked commit d-m-u@ffadf3f with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🏆

Copy link
Member

@romanblanco romanblanco left a comment

Choose a reason for hiding this comment

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

Tested in UI, fixes an issue I've ran into: 👍

F, [2018-01-11T15:24:48.047465 #27722] FATAL -- : Error caught: [NoMethodError] undefined method `cloud_network_id' for nil:NilClass 
/home/rblanco/.rbenv/versions/2.4.1/lib/ruby/gems/2.4.0/bundler/gems/manageiq-gems-pending-f793b3c6929e/lib/gems/pending/util/extensions/miq-object.rb:9:in `deep_send'
/home/rblanco/devel/manageiq/app/models/manageiq/providers/cloud_manager/provision_workflow.rb:121:in `get_targets_for_source'
/home/rblanco/devel/manageiq/app/models/manageiq/providers/cloud_manager/provision_workflow.rb:31:in `allowed_cloud_networks'
/home/rblanco/devel/manageiq/app/models/miq_request_workflow.rb:288:in `get_field'
/home/rblanco/devel/manageiq/app/models/miq_request_workflow.rb:275:in `block in get_all_fields'
/home/rblanco/devel/manageiq/app/models/miq_request_workflow.rb:275:in `each_key'
/home/rblanco/devel/manageiq/app/models/miq_request_workflow.rb:275:in `get_all_fields'
/home/rblanco/devel/manageiq/app/models/miq_request_workflow.rb:103:in `block in init_from_dialog'
/home/rblanco/devel/manageiq/app/models/miq_request_workflow.rb:102:in `each'
/home/rblanco/devel/manageiq/app/models/miq_request_workflow.rb:102:in `init_from_dialog'
/home/rblanco/devel/manageiq-ui-classic/app/controllers/application_controller/miq_request_methods.rb:835:in `prov_set_form_vars'
/home/rblanco/devel/manageiq-ui-classic/app/controllers/catalog_controller.rb:144:in `atomic_form_field_changed'

screencast from 2018-01-11 14-18-59

syncrou
syncrou approved these changes Jan 11, 2018
@d-m-u d-m-u changed the title Fixes logic for allowed_cloud_networks [WIP] Fixes logic for allowed_cloud_networks Jan 11, 2018
Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

After discussing with @d-m-u - We're looking at implementing the fix in the provider.

@miq-bot miq-bot added the wip label Jan 11, 2018
source = load_ar_obj(get_source_vm)
targets = get_targets_for_source(source, :cloud_filter, CloudNetwork, 'cloud_network_id')
allowed_ci(:cloud_network, [:availability_zone], targets.map(&:id))

if src_obj.all_cloud_networks.nil?
Copy link
Member

Choose a reason for hiding this comment

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

The all_cloud_networks call should always return an array since it is an association (here and here) or a combination of two arrays for a tenant.

Do you really want .blank? here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't always returning an array.

Copy link
Member

Choose a reason for hiding this comment

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

Can you provide a reference to where it would not? Is this just in a testing scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just in testing.

if src_obj.all_cloud_networks.nil?
return allowed_ci(:cloud_network, [:availability_zone], targets.map(&:id))
else
src_obj.all_cloud_networks.each_with_object({}) do |cn, hash|
Copy link
Member

Choose a reason for hiding this comment

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

This fixes the tests but is it really addressing the original issue of rbac filtering on this element?

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

drew and I consensus'd and have a suggestion for the following course of action:

  1. take code in main in recent PR and put into amazon
  2. revert main and leave it to pre-yesterday

and if still necessary at this point:
3) fix broken test
4) deal with Roman's stack trace

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

We think that this has been replaced with
#16806 and
https://github.com/ManageIQ/manageiq-providers-amazon/pull/391/files

@d-m-u
Copy link
Contributor Author

d-m-u commented Jan 11, 2018

Closing because the two PRs listed in comment above cover both UI issue and broken test in content repo.

@d-m-u d-m-u deleted the fixing_content_repo branch October 25, 2018 14:15
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.

5 participants