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

Removed provider_foreman explorer and added new non-explorer style screens under Configuration main tab. #19949

Merged
merged 16 commits into from
May 8, 2020

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented Mar 11, 2020

Removed provider_foreman explorer and added new non-explorer style screens under Configuration main tab.

Follow up PR for ManageIQ/manageiq-ui-classic#6730

before
before

before_shortcuts

after
Screenshot from 2020-04-09 12-43-04

after_shortcuts

@Fryguy
Copy link
Member

Fryguy commented Mar 11, 2020

We're going to need a data migration for the user roles and the shortcuts if the identifier is changing.

@h-kataria h-kataria force-pushed the provider_foreman_renaming branch from 77622f1 to 813b6ab Compare March 12, 2020 03:11
@h-kataria h-kataria removed the wip label Mar 12, 2020
@h-kataria h-kataria changed the title [WIP] - Renamed provider_foreman to configuration_manager Renamed provider_foreman to configuration_manager Mar 12, 2020
@h-kataria h-kataria force-pushed the provider_foreman_renaming branch 2 times, most recently from e4d4496 to 1d3d35e Compare March 20, 2020 22:15
@h-kataria h-kataria changed the title Renamed provider_foreman to configuration_manager Removed provider_foreman explorer and added new non-explorer style screens under Configuration main tab. Mar 25, 2020
@h-kataria
Copy link
Contributor Author

We're going to need a data migration for the user roles and the shortcuts if the identifier is changing.

Since old screens are removed and new screens are added in UI, these are no longer renaming of existing features.

@h-kataria h-kataria force-pushed the provider_foreman_renaming branch from f6ef583 to 0e201d3 Compare March 25, 2020 23:54
@skateman
Copy link
Member

@h-kataria travis issue seems relevant (loading the features YAML fails)

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

The Seal of Approval

@bdunne
Copy link
Member

bdunne commented Mar 30, 2020

We're going to need a data migration for the user roles and the shortcuts if the identifier is changing.

Since old screens are removed and new screens are added in UI, these are no longer renaming of existing features.

I think Jason's concern is for any existing users that have permissions to old identifiers, those need to be migrated to use the new permissions, right?

@skateman
Copy link
Member

skateman commented Apr 1, 2020

I'm a little confused by the seeding vs migrations, let's say I have this migration:

  def up
    old_feature = MiqProductFeature.find_by(:identifier => 'provider_foreman_explorer').id
    new_features = MiqProductFeature.where(:identifier => %w[configuration_manager_show_list configuration_profile_show_list configured_system_show_list]).pluck(:id)

    old_assignments = MiqRolesFeature.where(:miq_product_feature_id => old_feature)

    old_assignments.each do |a|
      new_features.each do |f_id|
        MiqRolesFeature.create(:miq_product_feature_id => f_id, :miq_user_role_id => a.user_role_id)
      end
    end

    old_assignments.delete_all
  end

I am replacing all foreman_explorers with the 3 new features in all user roles. However, when the db:seed runs through, the old_feature will not be available. So my question is how to proceed @Fryguy @bdunne

@Fryguy
Copy link
Member

Fryguy commented Apr 1, 2020

Easiest thing is to just rename the existing feature in the migration as long as the features' parents are still the same and nothing else changes. Otherwise you will need to create the new features in the migration as well.

Copy link
Member

@skateman skateman left a comment

Choose a reason for hiding this comment

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

I found some problems while writing the migration, also I'd like to propose the name ems_configuration if that's possible.

db/fixtures/miq_product_features.yml Outdated Show resolved Hide resolved
db/fixtures/miq_user_roles.yml Outdated Show resolved Hide resolved
@h-kataria
Copy link
Contributor Author

@h-kataria I don't see any cross-repo tests, unless I'm missing it. Can you create one?

Added cross-repo tests ManageIQ/manageiq-cross_repo-tests#113

@@ -46,6 +46,8 @@ class ConfiguredSystem < ApplicationRecord
scope :with_manager, ->(manager_id) { where(:manager_id => manager_id) }
scope :with_configuration_profile_id, ->(profile_id) { where(:configuration_profile_id => profile_id) }
scope :without_configuration_profile_id, -> { where(:configuration_profile_id => nil) }
scope :not_ansible, -> { where.not(:type => ["ManageIQ::Providers::EmbeddedAnsible::AutomationManager::ConfiguredSystem",
"ManageIQ::Providers::AnsibleTower::AutomationManager::ConfiguredSystem"]) }
Copy link
Member

@Fryguy Fryguy Apr 23, 2020

Choose a reason for hiding this comment

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

I really don't like this ☹️ ... I'll merge for now but we need a different way to do this.

cc @bdunne @agrare

Copy link
Member

Choose a reason for hiding this comment

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

ewww

Copy link
Member

Choose a reason for hiding this comment

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

This should really be something describing "not from automation managers", but not calling out ansible. Or perhaps in the positive, "under configuration managers only"

Copy link
Member

Choose a reason for hiding this comment

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

I'm tempted to actually make this a blocker...we really can't be adding references to plugins in core. @agrare Is there a way to express this without resorting to type checks?

Copy link
Member

Choose a reason for hiding this comment

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

Why are we excluding AutomationManager::ConfiguredSystems ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah we can check if the ext_management_system is a ConfigurationManager

Copy link
Member

@agrare agrare Apr 23, 2020

Choose a reason for hiding this comment

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

@Fryguy you might know a cleaner way of checking the type but something like this would do the trick:

scope :configuration_manager, -> { where(:manager => ManageIQ::Providers::ConfigurationManager.all) }
>> ConfiguredSystem.configuration_manager
  ConfiguredSystem Load (0.8ms)  SELECT  "configured_systems".* FROM "configured_systems" WHERE "configured_systems"."manager_id" IN (SELECT "ext_management_systems"."id" FROM "ext_management_systems" WHERE "ext_management_systems"."type" IN ('ManageIQ::Providers::ConfigurationManager', 'ManageIQ::Providers::Foreman::ConfigurationManager')) LIMIT $1  [["LIMIT", 11]]
  ConfiguredSystem Inst Including Associations (0.1ms - 1rows)
=> #<ActiveRecord::Relation [#<ConfiguredSystem id: 1, type: nil, hostname: nil, direct_operating_system_flavor_id: nil, configuration_profile_id: nil, manager_id: 7, manager_ref: nil, created_at: "2020-04-23 16:29:11", updated_at: "2020-04-23 16:29:11", last_checkin: nil, build_state: nil, configuration_location_id: nil, configuration_organization_id: nil, ipaddress: nil, mac_address: nil, ipmi_present: nil, puppet_status: nil, direct_customization_script_ptable_id: nil, direct_customization_script_medium_id: nil, operating_system_flavor_id: nil, customization_script_medium_id: nil, customization_script_ptable_id: nil, inventory_root_group_id: nil, virtual_instance_ref: nil, counterpart_type: nil, counterpart_id: nil>]>

Copy link
Member

@Fryguy Fryguy Apr 23, 2020

Choose a reason for hiding this comment

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

Why are we excluding AutomationManager::ConfiguredSystems ?

Technically the configured_systems records include records from both ConfigurationManagers as well as AutomationManagers. When viewing the list from the Configuration -> Configured Systems we need to exclude them.

Now that I've written that out, that is EXACTLY what we do with Cloud instances/templates vs Infra vms/templates, so whatever we do there is exactly what we should do here. @h-kataria How do we do that differentiation for Cloud/Infra vms?

Copy link
Member

@Fryguy Fryguy Apr 23, 2020

Choose a reason for hiding this comment

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

scope :configuration_manager, -> { where(:manager => ManageIQ::Providers::ConfigurationManager.all) }`

Minor, but I think I'd change that slightly to

scope :under_configuration_managers, -> { where(:manager => ManageIQ::Providers::ConfigurationManager.all) }`

Otherwise, :configuration_manager appears like a has_one relationship and would return a configuration_manager, but it doesn't

Copy link
Member

Choose a reason for hiding this comment

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

When viewing the list from the Configuration -> Configured Systems we need to exclude them.

That makes much more sense, we want to group them not exclude some

Minor, but I think I'd changed that slightly to

Oh yeah wasn't suggesting a name haha

Added named_scope to get Configuration Manager specific Configured System records. Ansible Configured systems are being displayed on Automation Manager explorer
@h-kataria h-kataria force-pushed the provider_foreman_renaming branch from 924044c to 21d8260 Compare April 23, 2020 20:00
locale/en.yml Outdated
@@ -781,9 +781,11 @@ en:
ManageIQ::Providers::AnsibleTower::AutomationManager::ConfigurationScript: Job Template (Ansible Tower)
ManageIQ::Providers::AnsibleTower::AutomationManager::ConfigurationWorkflow: Workflow Template (Ansible Tower)
ManageIQ::Providers::AnsibleTower::AutomationManager::Playbook: Playbook (Ansible Tower)
ManageIQ::Providers::CloudAutomationManager::ConfigurationManager: Configuration Manager (Cloud Automation Manager)
Copy link
Member

Choose a reason for hiding this comment

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

These are pluggable now that we have #20100 merged, so the provider plugins should bring their own dictionary entries. cc @agrare I think we should blast out these changes to all providers.

locale/en.yml Outdated Show resolved Hide resolved
@Fryguy
Copy link
Member

Fryguy commented Apr 28, 2020

@h-kataria This PR looks good to me once we remove the dictionary entries.

These entries are no longer needed after merge of ManageIQ#20100
@miq-bot
Copy link
Member

miq-bot commented Apr 29, 2020

Checked commits h-kataria/manageiq@1a2ae17~...25bee8a with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
2 files checked, 0 offenses detected
Everything looks fine. 🍰

@Fryguy Fryguy merged commit 431bbe3 into ManageIQ:master May 8, 2020
@simaishi
Copy link
Contributor

@h-kataria Backporting to jansa conflicts on db/fixtures/miq_product_features.yml. Can you please create a PR for jansa branch?

h-kataria added a commit to h-kataria/manageiq that referenced this pull request May 18, 2020
Renamed provider_foreman specific features, links etc to configuration_manager
Jansa specific PR for backport of ManageIQ#19949
@h-kataria
Copy link
Contributor Author

h-kataria commented May 18, 2020

@simaishi created Jansa specific PR #20183

simaishi pushed a commit that referenced this pull request May 21, 2020
Removed provider_foreman explorer and added new non-explorer style screens under Configuration main tab.

(cherry picked from commit 431bbe3)
@simaishi
Copy link
Contributor

Jansa backport details:

$ git log -1
commit f8b58af7f4902716b571ce7dbe6805d2bdd9805c
Author: Jason Frey <[email protected]>
Date:   Thu May 7 22:05:54 2020 -0400

    Merge pull request #19949 from h-kataria/provider_foreman_renaming

    Removed provider_foreman explorer and added new non-explorer style screens under Configuration main tab.

    (cherry picked from commit 431bbe358c8f8858d6766619c697d5dbb497ca07)

@h-kataria h-kataria deleted the provider_foreman_renaming branch December 31, 2020 15:47
NickLaMuro added a commit to NickLaMuro/manageiq-ui-service that referenced this pull request Jul 30, 2021
These permissions where changed here:

ManageIQ/manageiq#19949

and as such, the Ansible playbook results would not be displayed.

Changing to use "service_view" permission since the manageiq-ui-classic
uses "service" (level above), and this is only displaying the view, so
this should be the only permission required.
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.

9 participants