Fixes #23706 - CV/LE show the right value on Host form #7398

dLobatog · 2018-05-28T12:24:26Z

Before this patch, the following situation could happen:

Host A is part of a Host Group 'test'. Host A inherits LE/CV 'test'
coming from Host Group 'test'
We decide to change the LE/CV of Host A to 'production', either
through the API or the UI.
The next time we decide to check the UI, the helper was wrong (this
patch fixes it) and showed the LE/CV from the Host Group 'test'. This is
very confusing and may even cause you to change the LE/CV to from
'production' to 'test' by accident.

In my opinion this should be backported to 3.6 at least

🎥

Before: https://webmshare.com/q5g6Q
After: https://webmshare.com/mPa3D

theforeman-bot · 2018-05-28T12:24:31Z

Issues: #23706

ares · 2018-05-29T10:29:04Z

Makes sense to me, hostgroup already implements both methods (via hostgroup extensions), reporter confirms it fixes the issue, 👍 from me

ares · 2018-05-29T14:02:43Z

@dLobatog the test failure is related

Failure:

HostAndHostGroupsHelperLifecycleEnvironmentTests#test_accessible_lifecycle_environments_limited [/var/lib/workspace/workspace/katello-pr-test/test/helpers/hosts_and_hostgroups_helper_test.rb:31]:

--- expected

+++ actual

@@ -1 +1 @@

-[#<Katello::KTEnvironment id: 562075838, name: "Library", description: "This is the Library", library: true, organization_id: 114267492, created_at: "2018-05-28 12:33:33", updated_at: "2018-05-28 12:33:33", label: "library_label", registry_name_pattern: nil>]

+#<ActiveRecord::AssociationRelation []>

dLobatog

@ares Updated, had to jump through some hoops to get the test passing, just some differences between working with FactoryBot vs real models saved in the db.

dLobatog · 2018-06-01T10:28:45Z

test/helpers/hosts_and_hostgroups_helper_test.rb

@@ -26,6 +29,8 @@ def test_accessible_lifecycle_environments
  end

  def test_accessible_lifecycle_environments_limited
+    @host.save


Had to do this so I could call @host.lifecycle_environment later..
On the test environment with FactoryBot.build it doesn't work as the association needs to be saved in the database to work: https://github.com/Katello/katello/blob/master/app/models/katello/concerns/content_facet_host_extensions.rb#L29

ares · 2018-06-04T11:46:15Z

could some committer take a look and merge if ACKed?

ehelms · 2018-06-07T14:45:11Z

@parthaa Can you take a look?

jlsherrill · 2018-06-13T19:49:09Z

@dLobatog this doesn't seem to fix the issue? at least i can reproduce it as described:

change the LE/CV or content-source from WebUI and save it
click on edit host again and LE/CV changed to the default one in the hostgroup

jlsherrill · 2018-06-14T17:04:58Z

app/helpers/katello/hosts_and_hostgroups_helper.rb

@@ -77,20 +61,19 @@ def kickstart_repository_id(host, options = {})
    end

    def fetch_lifecycle_environment(host, options = {})
+      return host.lifecycle_environment if host.lifecycle_environment.present?
      selected_host_group = options.fetch(:selected_host_group, nil)
      return lifecycle_environment(selected_host_group) if selected_host_group.present?


looks like you deleted the lifecycle_environment method above:

undefined method `lifecycle_environment' for #<#Class:0x0000000bb7e3e0:0x0000000bb8ed58>

this seems to happen if you create a host, select the host group, but don't select a CV or LCE. Then go to the host edit page.

It does seem to solve the original issue thou gh.

parthaa · 2018-06-15T12:51:02Z

app/helpers/katello/hosts_and_hostgroups_helper.rb

    end

    def fetch_content_view(host, options = {})
+      return host.content_view if host.content_view.present?
      selected_host_group = options.fetch(:selected_host_group, nil)
      return content_view(selected_host_group) if selected_host_group.present?


This method has been deleted as well. This line needs to change too

dLobatog

@parthaa @jlsherrill Updated to fix the calls to the removed method

Before this patch, the following situation could happen: 1. Host A is part of a Host Group 'test'. Host A inherits LE/CV 'test' coming from Host Group 'test' 2. We decide to change the LE/CV of Host A to 'production', either through the API or the UI. 3. The next time we decide to check the UI, the helper was wrong (this patch fixes it) and showed the LE/CV from the Host Group 'test'. This is very confusing and may even cause you to change the LE/CV to from 'production' to 'test' by accident.

dLobatog

@jlsherrill @parthaa Green tests 😄

If you could take a look again it'd be great, it's really bothering the people who opened the BZ (it has several cases attached as you may see).

akofink

It works as expected, though I noticed this scenario:

create a host
assign a CV/LE and save, leaving hostgroup empty
edit the host, select a host group

At this point, I can't clear the CV or LE without incurring validation errors. There doesn't seem to be a way to tell Foreman to use the CV/LE associated with the host group. This is kind of a contrived scenario, but users may encounter it. What do you think? @jlsherrill had mentioned to me that we might think about something like the inherit button we use for the puppet fields.

jlsherrill · 2018-07-13T14:10:50Z

@akofink i agree, but i think we might should open a 2nd issue for that? This would be a better change for the near term and less risky to backport to older releases. Thoughts?

akofink · 2018-07-13T14:48:22Z

I'm good with that. I'll file an issue for it.

https://projects.theforeman.org/issues/24253

akofink

ACK from me! Thanks @dLobatog!

jlsherrill · 2018-07-13T15:09:19Z

Thanks @dLobatog and @akofink !

theforeman-bot added Needs testing Not yet reviewed labels May 28, 2018

dLobatog force-pushed the 23706 branch from 274b7b9 to 5592fa0 Compare June 1, 2018 10:27

dLobatog commented Jun 1, 2018

View reviewed changes

jlsherrill reviewed Jun 14, 2018

View reviewed changes

parthaa reviewed Jun 15, 2018

View reviewed changes

dLobatog force-pushed the 23706 branch from 5592fa0 to 9fb2f3a Compare June 15, 2018 13:30

dLobatog commented Jun 15, 2018

View reviewed changes

dLobatog force-pushed the 23706 branch from 9fb2f3a to f9ac4b5 Compare July 11, 2018 14:48

dLobatog commented Jul 11, 2018

View reviewed changes

akofink mentioned this pull request Jul 12, 2018

Fixes #24131: Use saved LE/CV instead of HG's #7490

Closed

akofink reviewed Jul 12, 2018

View reviewed changes

akofink approved these changes Jul 13, 2018

View reviewed changes

jlsherrill merged commit 81ecd36 into Katello:master Jul 13, 2018

dLobatog mentioned this pull request Jul 17, 2018

Refs #23706 - Show proper content source #7533

Merged

Fixes #23706 - CV/LE show the right value on Host form #7398

Fixes #23706 - CV/LE show the right value on Host form #7398

dLobatog commented May 28, 2018 •

edited

theforeman-bot commented May 28, 2018

ares commented May 29, 2018

ares commented May 29, 2018

dLobatog left a comment

dLobatog Jun 1, 2018

ares commented Jun 4, 2018

ehelms commented Jun 7, 2018

jlsherrill commented Jun 13, 2018

jlsherrill Jun 14, 2018

jlsherrill Jun 14, 2018

parthaa Jun 15, 2018

dLobatog left a comment

dLobatog left a comment

akofink left a comment

jlsherrill commented Jul 13, 2018

akofink commented Jul 13, 2018 •

edited

akofink left a comment

jlsherrill commented Jul 13, 2018

Fixes #23706 - CV/LE show the right value on Host form #7398

Fixes #23706 - CV/LE show the right value on Host form #7398

Conversation

dLobatog commented May 28, 2018 • edited

theforeman-bot commented May 28, 2018

ares commented May 29, 2018

ares commented May 29, 2018

dLobatog left a comment

Choose a reason for hiding this comment

dLobatog Jun 1, 2018

Choose a reason for hiding this comment

ares commented Jun 4, 2018

ehelms commented Jun 7, 2018

jlsherrill commented Jun 13, 2018

jlsherrill Jun 14, 2018

Choose a reason for hiding this comment

jlsherrill Jun 14, 2018

Choose a reason for hiding this comment

parthaa Jun 15, 2018

Choose a reason for hiding this comment

dLobatog left a comment

Choose a reason for hiding this comment

dLobatog left a comment

Choose a reason for hiding this comment

akofink left a comment

Choose a reason for hiding this comment

jlsherrill commented Jul 13, 2018

akofink commented Jul 13, 2018 • edited

akofink left a comment

Choose a reason for hiding this comment

jlsherrill commented Jul 13, 2018

dLobatog commented May 28, 2018 •

edited

akofink commented Jul 13, 2018 •

edited