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

Added get_targets_for_ base class methods #16707

Merged
merged 2 commits into from
Jan 8, 2018

Conversation

syncrou
Copy link
Contributor

@syncrou syncrou commented Dec 20, 2017

Turns on RBAC filtering by default at the base class level for all allowed_ methods_ in the ManageIQ::CloudManager class.

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

Steps for Testing/QA

  1. Add tags to a user for use on a Cloud provider*
  2. Login in as that user and Provision an Instance
  3. Only Tagged objects should appear in the dropdowns

There are specific providers that have already overridden the Base Class methods and in some cases the overridden methods do not yet support RBAC filtering. e.g. Amazon and the allowed_floating_ips method.

@syncrou
Copy link
Contributor Author

syncrou commented Dec 20, 2017

@miq-bot assign @gmcculloug

@miq-bot add_label providers, bz, blocker, wip

@miq-bot
Copy link
Member

miq-bot commented Dec 20, 2017

@syncrou Cannot apply the following label because they are not recognized: bz

@miq-bot miq-bot changed the title Added get_targets_for_ base class methods [WIP] Added get_targets_for_ base class methods Dec 20, 2017
@syncrou
Copy link
Contributor Author

syncrou commented Dec 20, 2017

@miq-bot add_label bug

@miq-bot miq-bot added the bug label Dec 20, 2017
@syncrou
Copy link
Contributor Author

syncrou commented Dec 21, 2017

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Added get_targets_for_ base class methods Added get_targets_for_ base class methods Dec 21, 2017
@miq-bot miq-bot removed the wip label Dec 21, 2017
allow_any_instance_of(User).to receive(:get_timezone).and_return(Time.zone)
allow_any_instance_of(ManageIQ::Providers::CloudManager::ProvisionWorkflow).to receive(:update_field_visibility)
wf = described_class.new({:src_vm_id => template.id}, admin.userid)
wf
Copy link
Member

Choose a reason for hiding this comment

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

Saw this on the provider version of the spec. Drop the wf variable and just let described_class.new return the object.

let(:workflow) do
stub_dialog
allow_any_instance_of(User).to receive(:get_timezone).and_return(Time.zone)
allow_any_instance_of(ManageIQ::Providers::CloudManager::ProvisionWorkflow).to receive(:update_field_visibility)
Copy link
Member

Choose a reason for hiding this comment

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

Replace ManageIQ::Providers::CloudManager::ProvisionWorkflow with described_class.


let(:admin) { FactoryGirl.create(:user_with_group) }
let(:ems) { FactoryGirl.create(:ems_amazon) }
let(:template) { FactoryGirl.create(:template_amazon, :name => "template", :ext_management_system => ems) }
Copy link
Member

Choose a reason for hiding this comment

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

Can we use generic factories where possible. Example: template_cloud.

This we need a little more work before we can replace all of them.

@syncrou syncrou force-pushed the tag_filter_cloud_stuffs branch 5 times, most recently from f310da7 to a4de244 Compare December 21, 2017 22:15
@@ -3,6 +3,8 @@
sequence(:name) { |n| "availability_zone_#{seq_padded_for_sorting(n)}" }
end

factory :availability_zone_generic, :parent => :availability_zone
Copy link
Member

Choose a reason for hiding this comment

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

@syncrou The tests pass locally if I just use availability_zone in the tests. Is this needed for some reason?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmcculloug - I'll do more testing - I was seeing failures without the :parent value in place.

1. Turns on RBAC filtering by default at the base class level for all allowed_ methods in the CloudManager class

https://bugzilla.redhat.com/show_bug.cgi?id=1514237
@miq-bot
Copy link
Member

miq-bot commented Jan 8, 2018

Some comments on commits syncrou/manageiq@14830a7~...449f2ee

spec/models/manageiq/providers/cloud_manager/provision_workflow_spec.rb

  • ⚠️ - 10 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.
  • ⚠️ - 9 - Detected allow_any_instance_of. This RSpec method is highly discouraged, please only use when absolutely necessary.

@miq-bot
Copy link
Member

miq-bot commented Jan 8, 2018

Checked commits syncrou/manageiq@14830a7~...449f2ee with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@syncrou syncrou closed this Jan 8, 2018
@syncrou syncrou reopened this Jan 8, 2018
@gmcculloug gmcculloug merged commit 7783ec1 into ManageIQ:master Jan 8, 2018
@gmcculloug gmcculloug added this to the Sprint 77 Ending Jan 15, 2018 milestone Jan 8, 2018
simaishi pushed a commit that referenced this pull request Jan 9, 2018
@simaishi
Copy link
Contributor

simaishi commented Jan 9, 2018

Gaprindashvili backport details:

$ git log -1
commit 3c5088d165321c2043dedf0c521872e5389823cf
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Jan 8 18:05:39 2018 -0500

    Merge pull request #16707 from syncrou/tag_filter_cloud_stuffs
    
    Added get_targets_for_ base class methods
    (cherry picked from commit 7783ec1d9839ffffe1a915e794e3d75b40f40e0d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1532646

simaishi pushed a commit that referenced this pull request Jan 10, 2018
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 271d75c8a3e5fa6e8cdeff926dfda9ba3028c38a
Author: Greg McCullough <gmccullo@redhat.com>
Date:   Mon Jan 8 18:05:39 2018 -0500

    Merge pull request #16707 from syncrou/tag_filter_cloud_stuffs
    
    Added get_targets_for_ base class methods
    (cherry picked from commit 7783ec1d9839ffffe1a915e794e3d75b40f40e0d)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1533139

d-m-u pushed a commit to d-m-u/manageiq that referenced this pull request Jun 6, 2018
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.

4 participants