Create the provider when creating the config_manager #62

agrare · 2020-05-13T20:19:49Z

Stay consistent with the rest of the managers which create the "primary" manager and auto-creates the provider.

This will allow the UI to add ConfigurationManagers the same way they add everything else (permissions and all) which will enable allowing us to add other types of ConfigurationManagers.

agrare · 2020-05-13T20:21:31Z

cc @skateman this should let us use the DDF create_from_params on the Foreman::ConfigurationManager

app/models/manageiq/providers/foreman/configuration_manager.rb

skateman · 2020-05-14T08:38:44Z

This suffix building in the name is causing a lot of headache. Do we really need it? When I try to edit a newly created provider, it won't set the name of the Provider but the Manager so it will head to inconsistencies in the names. We could solve it by delegating the name= to the Provider but as the API provides the name of the Manager we would be setting the suffixed name on the Provider which triggers the ensure_manager and sets a double suffix on the Manager.

It adds to the confusion that ensure_provider calls the ensure_manager and they are both setting values. We should probably just use one of them and my choice is the ensure_provider as that would be the new way of adding configuration managers. This doesn't solve the problem with the suffix, but at least would decrease the complexity of the code.

If we really want to keep the suffixing, we should probably add checks that ensure that the suffix is just added once in the Manager and it's not being added to the Provider.

skateman · 2020-05-14T09:01:18Z

I created a UI PR that allows you to run the new provider forms for the Configuration Managers and adjusted the DDF schema to make the params validation on the API side pass. These should allow you to try some things:
ManageIQ/manageiq-ui-classic#7047
#60

Note that after applying your changes there's an exception in the old forms when editing a provider, but it doesn't block you from working with the new form.

agrare · 2020-05-14T21:14:49Z

Okay this now updates the provider name when the manager name is changed. I want to keep the old way (creating and updating the provider) as well as the DDF forms way (creating and updating the Configuration Manager) working until we officially move over to DDF. Then we can get rid of the Provider#ensure_managers and ConfigurationManager#ensure_provider and just use one but for now both routes work even if it seems like overkill.

skateman

I like the idea, but I have some concerns about the naming. Also when I edit a manager, it won't update the name of the provider, just the manager. I guess you'd need to delegate the name= to the provider.

Also when requesting endpoints and authentications through the API, I am getting empty arrays:

// GET /api/providers/ID\?attributes\=authentications,endpoints
{
  // ...
  "authentications": [],
  "endpoints": [],
  // ...
}

app/models/manageiq/providers/foreman/configuration_manager.rb

app/models/manageiq/providers/foreman/provider.rb

agrare · 2020-05-15T14:26:49Z

Also when I edit a manager, it won't update the name of the provider, just the manager.

Actually it does update the provider name if you edit the manager and vice versa

>> foreman_c_manager = ManageIQ::Providers::Foreman::ConfigurationManager.first
=> #<ManageIQ::Providers::Foreman::Provider id: 25, type: "ManageIQ::Providers::Foreman::Provider", name: "Foreman", guid: "66f4c9f4-f992-41bb-919a-2ecd214c0515", zone_id: 2, created_at: "2020-05-15 14:21:20", updated_at: "2020-05-15 14:21:20", tenant_id: 1>
=> "Foreman Configuration Manager"
>> foreman_c_manager.provider.name
=> "Foreman"
>> foreman_c_manager.name
=> "Foreman Configuration Manager"
>> foreman_c_manager.name = "Foreman Updated Configuration Manager"
>> foreman_c_manager.save!
=> true
>> foreman_c_manager.provider.name
=> "Foreman Updated"
>> foreman_c_manager.name
=> "Foreman Updated Configuration Manager"
>> foreman_c_manager.provider.name = "Foreman"
>> foreman_c_manager.provider.save!
=> true
>> foreman_c_manager.name
>> foreman.name
=> "Foreman Configuration Manager"

agrare · 2020-05-15T14:30:46Z

Also when requesting endpoints and authentications through the API, I am getting empty arrays:

Really? That's not what I'm seeing:

$ curl --user admin:smartvm http://localhost:3000/api/providers/63\?attributes\=authentications,endpoints 2>/dev/null | python -m json.tool
{
    "api_version": null,
    "authentications": [
        {
            "authtype": "default",
            "id": "19",
            <clip>
            "type": "AuthUseridPassword",
            "updated_on": "2020-05-15T14:21:21Z",
            "userid": "admin"
        }
    ],
    "created_on": "2020-05-15T14:21:20Z",
    "enabled": true,
    "endpoints": [
        {
            <clip>
            "resource_id": "25",
            "resource_type": "Provider",
            "role": "default",
            "security_protocol": null,
            "updated_at": "2020-05-15T14:21:20Z",
            "url": "https://localhost",
            "verify_ssl": 0
        }
    ],
    "guid": "bc31901d-2738-4ca3-b3b7-8be638fba838",
    "host_default_vnc_port_end": null,
    "host_default_vnc_port_start": null,
    "href": "http://localhost:3000/api/providers/63",
    "id": "63",
     "name": "Foreman Configuration Manager",
    "provider_id": "25",
    "type": "ManageIQ::Providers::Foreman::ConfigurationManager",
    "uid_ems": null,
}

skateman · 2020-05-18T13:22:32Z

Also when I edit a manager, it won't update the name of the provider, just the manager.

Actually it does update the provider name if you edit the manager and vice versa

>> foreman_c_manager = ManageIQ::Providers::Foreman::ConfigurationManager.first
=> #<ManageIQ::Providers::Foreman::Provider id: 25, type: "ManageIQ::Providers::Foreman::Provider", name: "Foreman", guid: "66f4c9f4-f992-41bb-919a-2ecd214c0515", zone_id: 2, created_at: "2020-05-15 14:21:20", updated_at: "2020-05-15 14:21:20", tenant_id: 1>
=> "Foreman Configuration Manager"
>> foreman_c_manager.provider.name
=> "Foreman"
>> foreman_c_manager.name
=> "Foreman Configuration Manager"
>> foreman_c_manager.name = "Foreman Updated Configuration Manager"
>> foreman_c_manager.save!
=> true
>> foreman_c_manager.provider.name
=> "Foreman Updated"
>> foreman_c_manager.name
=> "Foreman Updated Configuration Manager"
>> foreman_c_manager.provider.name = "Foreman"
>> foreman_c_manager.provider.save!
=> true
>> foreman_c_manager.name
>> foreman.name
=> "Foreman Configuration Manager"

Maybe from the console, but not if I do the update via the API 😕 we're using ems.assign_attributes(params) and ems.save!.

app/models/manageiq/providers/foreman/configuration_manager.rb

Fryguy · 2020-05-18T15:54:01Z

we're using ems.assign_attributes(params) and ems.save!.

ems.save! should do the part where it updates the provider...are you not seeing that happen?

skateman · 2020-05-18T17:31:09Z

@Fryguy yeah, that's what I'm saying. You can try with the 2 PRs mentioned here

skateman · 2020-05-28T19:46:33Z

ManageIQ/manageiq#18818

skateman · 2020-05-29T07:45:41Z

@agrare could you please rebase this, so @h-kataria test it without the issue?

skateman

We're facing some delegation errors for endpoints/authentications when editing a provider. Also changing the name (and I guess also the zone) of the manager doesn't prevail on the provider with the before_save callback. The code is a little chaotic as there are before callbacks in both models. I understand that you'd like to keep the old behavior for compatibility, so here's my idea:

Let's define a name= method that removes the Configuration Manager suffix from the name and sets it on the provider, imagine it as a delegate_with_a_replace. The zone and zone= methods should be delegated normally and then we can strip down the ensure_provider to the absolute minimum and get rid of the ˙ensure_provider_name_and_zone` method.

We would need to change the ensure_managers method on the other side, to propagate back the suffixed name as the before_validation callback doesn't get called upon an edit.

app/models/manageiq/providers/foreman/configuration_manager.rb

skateman · 2020-05-30T09:27:20Z

app/models/manageiq/providers/foreman/configuration_manager.rb

           :verify_credentials,
           :with_provider_connection,
           :to => :provider

+  belongs_to :provider, :autosave => true, :dependent => :destroy
+
+  before_save :ensure_provider_name_and_zone


The before_save doesn't really deal with the new name and zone value coming from an edit action. I debugged the method below and the name value was the one stored in the DB and not the one I'm changing the value to. It works well on create, but not when editing through the API.

skateman · 2020-06-24T14:37:20Z

app/models/manageiq/providers/foreman/configuration_manager.rb

+    "#{provider.name} Configuration Manager"
+  end
+
+  delegate :name=, :zone, :zone=, :zone_id=, :to => :provider


You'd also need zone_id here so the API returns the correct info.

skateman · 2020-06-24T14:47:17Z

It's almost working, there's a slight issue with the validation of the provider URL when editing. I'll investigate...

agrare · 2020-06-24T17:41:12Z

@skateman yeah we weren't delegating endpoints= and authentications= so it was just destroying those on edit

agrare · 2020-06-24T17:42:53Z

There still seems to be an issue with verify_ssl always being 1, looks like the DDF form is saying it is a boolean checkbox but it really is an integer but that's not an issue with this PR as far as I can tell

agrare · 2020-06-25T11:32:33Z

I think the issue is that the provider isn't also saved with the ems when using .assign_attributes(:name => "").

When I do:

ems.name = "abcd"
ems.save!

then it works, but

ems.assign_attributes(:name => "abcd")
ems.save!

Not so much.

skateman · 2020-06-25T11:41:04Z

Can we force the save of the provider when the EMS is being saved?

agrare · 2020-06-25T12:36:03Z

That's what the autosave should be doing, we could explicitly save the provider if we override edit_with_params which is a little gross

skateman · 2020-06-25T12:37:36Z

That's what the autosave should be doing, we could explicitly save the provider if we override edit_with_params which is a little gross

I'd like to avoid a hack like this if possible 😕

skateman · 2020-07-01T09:24:03Z

Works well with ManageIQ/manageiq#20318

skateman

This is far from ideal, but the simplest way to ensure that changes to the foreman provider get saved when edited from the API

miq-bot · 2020-07-01T18:11:33Z

Checked commits agrare/manageiq-providers-foreman@8141a03~...cfaaa5b with ruby 2.5.7, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. 🍪

agrare · 2020-07-01T18:43:44Z

app/models/manageiq/providers/foreman/configuration_manager.rb

+        ems.save!
+      end
+    end
+  end


I've been having a lot of issues with getting the provider autosave working across all the different provider repos. I want to unblock @skateman while we work those out and the simplest way to do that is to manually save the provider while creating/editing the configuration_manager via the API

This is ugly, but if it works, I'm good with that.

agrare requested a review from Fryguy as a code owner May 13, 2020 20:19

chessbyte added the enhancement label May 13, 2020

agrare commented May 13, 2020

View reviewed changes

app/models/manageiq/providers/foreman/configuration_manager.rb Outdated Show resolved Hide resolved

agrare force-pushed the auto_create_provider_for_configuration_manager branch from c12df78 to b115261 Compare May 13, 2020 20:34

skateman mentioned this pull request May 14, 2020

Replace old angular config provider form with the new ProviderForm ManageIQ/manageiq-ui-classic#7047

Merged

agrare force-pushed the auto_create_provider_for_configuration_manager branch from b115261 to af1da58 Compare May 14, 2020 13:36

gtanzillo assigned Fryguy May 14, 2020

agrare force-pushed the auto_create_provider_for_configuration_manager branch from af1da58 to 451da64 Compare May 14, 2020 20:58

skateman suggested changes May 15, 2020

View reviewed changes

app/models/manageiq/providers/foreman/configuration_manager.rb Outdated Show resolved Hide resolved

app/models/manageiq/providers/foreman/provider.rb Outdated Show resolved Hide resolved

agrare force-pushed the auto_create_provider_for_configuration_manager branch from c2ffb65 to e94e1b7 Compare May 15, 2020 14:57

Fryguy reviewed May 18, 2020

View reviewed changes

app/models/manageiq/providers/foreman/configuration_manager.rb Show resolved Hide resolved

skateman suggested changes May 30, 2020

View reviewed changes

agrare force-pushed the auto_create_provider_for_configuration_manager branch 2 times, most recently from 96c94d1 to 27d56b7 Compare June 24, 2020 13:58

skateman reviewed Jun 24, 2020

View reviewed changes

agrare force-pushed the auto_create_provider_for_configuration_manager branch from ef4c33a to 452a512 Compare June 24, 2020 17:33

skateman mentioned this pull request Jun 26, 2020

Replace SSL verification checkbox with a dropdown in the DDF #65

Merged

agrare added 10 commits June 30, 2020 08:31

Create the provider when creating the config_manager

8141a03

Dependent destroy the provider on deletion

b336fc1

Update provider name and zone when manager changes

418cbbd

Safeguards around naming and suffixes

f2c0058

Make ensure_provider* methods private

422dda2

Override name and delegate name= to provider

34fcf8f

Strip suffix when setting provider name

bc4d53b

Delegate zone_id to provider

056b786

Delegate endpoints=/authentications= to provider

9bc939a

Can't dependent destroy in all directions

ccb02da

agrare force-pushed the auto_create_provider_for_configuration_manager branch 2 times, most recently from 70ca2df to e0aad78 Compare June 30, 2020 12:59

Add specs for create_from_params and edit_with_params

dc9feda

agrare force-pushed the auto_create_provider_for_configuration_manager branch from e0aad78 to dc9feda Compare June 30, 2020 13:01

agrare mentioned this pull request Jun 30, 2020

Test autosave provider ManageIQ/manageiq-cross_repo-tests#148

Closed

skateman approved these changes Jul 1, 2020

View reviewed changes

Override create_from_params/edit_with_params to save provider

cfaaa5b

This is far from ideal, but the simplest way to ensure that changes to the foreman provider get saved when edited from the API

agrare commented Jul 1, 2020

View reviewed changes

gtanzillo approved these changes Jul 2, 2020

View reviewed changes

gtanzillo merged commit 65634b0 into ManageIQ:master Jul 2, 2020

agrare deleted the auto_create_provider_for_configuration_manager branch July 2, 2020 12:32

skateman mentioned this pull request Jul 2, 2020

Adjusted schema for DDF + validation ManageIQ/manageiq-providers-ansible_tower#228

Merged

agrare mentioned this pull request Jul 2, 2020

Fix foreman provider missing url in specs ManageIQ/manageiq#20330

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create the provider when creating the config_manager #62

Create the provider when creating the config_manager #62

agrare commented May 13, 2020 •

edited

Loading

agrare commented May 13, 2020

skateman commented May 14, 2020

skateman commented May 14, 2020

agrare commented May 14, 2020

skateman left a comment

agrare commented May 15, 2020

agrare commented May 15, 2020 •

edited

Loading

skateman commented May 18, 2020 •

edited

Loading

Fryguy commented May 18, 2020

skateman commented May 18, 2020

skateman commented May 28, 2020

skateman commented May 29, 2020

skateman left a comment

skateman May 30, 2020

skateman Jun 24, 2020

skateman commented Jun 24, 2020

agrare commented Jun 24, 2020

agrare commented Jun 24, 2020

agrare commented Jun 25, 2020

skateman commented Jun 25, 2020

agrare commented Jun 25, 2020

skateman commented Jun 25, 2020

skateman commented Jul 1, 2020

skateman left a comment

miq-bot commented Jul 1, 2020

agrare Jul 1, 2020

skateman Jul 2, 2020

Create the provider when creating the config_manager #62

Create the provider when creating the config_manager #62

Conversation

agrare commented May 13, 2020 • edited Loading

agrare commented May 13, 2020

skateman commented May 14, 2020

skateman commented May 14, 2020

agrare commented May 14, 2020

skateman left a comment

Choose a reason for hiding this comment

agrare commented May 15, 2020

agrare commented May 15, 2020 • edited Loading

skateman commented May 18, 2020 • edited Loading

Fryguy commented May 18, 2020

skateman commented May 18, 2020

skateman commented May 28, 2020

skateman commented May 29, 2020

skateman left a comment

Choose a reason for hiding this comment

skateman May 30, 2020

Choose a reason for hiding this comment

skateman Jun 24, 2020

Choose a reason for hiding this comment

skateman commented Jun 24, 2020

agrare commented Jun 24, 2020

agrare commented Jun 24, 2020

agrare commented Jun 25, 2020

skateman commented Jun 25, 2020

agrare commented Jun 25, 2020

skateman commented Jun 25, 2020

skateman commented Jul 1, 2020

skateman left a comment

Choose a reason for hiding this comment

miq-bot commented Jul 1, 2020

agrare Jul 1, 2020

Choose a reason for hiding this comment

skateman Jul 2, 2020

Choose a reason for hiding this comment

agrare commented May 13, 2020 •

edited

Loading

agrare commented May 15, 2020 •

edited

Loading

skateman commented May 18, 2020 •

edited

Loading