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

Fixes #36462 - check for presence of content facet before assigning CVE #10602

Merged
merged 2 commits into from
Jun 16, 2023

Conversation

jeremylenz
Copy link
Member

@jeremylenz jeremylenz commented Jun 13, 2023

What are the changes introduced in this pull request?

Sometimes in Katello we assume that all hosts have content facets. We should stop doing that.

Specifically in this case, the host edit page will try to inherit content facet attributes from a hostgroup without first checking if the host has a content facet.

Before assigning a content view environment based on the deprecated content_view_id and lifecycle_environment_id attributes, we should check if the content facet actually exists.

Considerations taken when implementing this change?

same as #10595 and #10600 ugh I'm getting tired of saying this

What are the testing steps for this pull request?

Create a hostgroup with CV and LCE
Create a host (Hosts > Create Host) - I did it by syncing a kickstart repo
In rails console, remove the host's content facet (verify that myhost.content_facet returns nil)
Edit the host and attempt to add/remove the hostgroup (you may have to fill in other required fields, it's a pain I know sorry)

Before patch:

NoMethodError (undefined method `assign_single_environment' for nil:NilClass):
13:38:08 rails.1   |  a198b8f9 |   
13:38:08 rails.1   |  a198b8f9 | /home/vagrant/katello/app/models/katello/concerns/host_managed_extensions.rb:14:in `update'

After patch: host is edited without error

@theforeman-bot
Copy link

Issues: #36462

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

Works like a charm. Do you think adding any tests make sense to look out for regressions?

@jeremylenz
Copy link
Member Author

Do you think adding any tests make sense to look out for regressions?

I did think about adding tests. But once the multi-CV feature is finished, a lot of this logic will be quite different anyway. Since the ubiquity of assign_single_environment is (hopefully) temporary, I wasn't sure if it was worth the effort. Happy to add some if you think so, though.

Copy link
Member

@ianballou ianballou left a comment

Choose a reason for hiding this comment

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

All good, if this is taken out then we shouldn't have regressions.

Copy link
Member Author

@jeremylenz jeremylenz left a comment

Choose a reason for hiding this comment

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

Added a test in hosts_controller_test 👍

app/models/katello/concerns/host_managed_extensions.rb Outdated Show resolved Hide resolved
@jeremylenz jeremylenz merged commit 2fdd82a into Katello:master Jun 16, 2023
4 of 5 checks passed
lfu pushed a commit to lfu/katello that referenced this pull request Jun 21, 2023
…VE (Katello#10602)

* Fixes #36462 - check for presence of content facet
  before assigning CVE

(cherry picked from commit 2fdd82a)
lfu pushed a commit that referenced this pull request Jun 21, 2023
…VE (#10602)

* Fixes #36462 - check for presence of content facet
  before assigning CVE

(cherry picked from commit 2fdd82a)
wbclark pushed a commit to wbclark/katello that referenced this pull request Sep 7, 2023
…VE (Katello#10602)

* Fixes #36462 - check for presence of content facet
  before assigning CVE

(cherry picked from commit 2fdd82a)
wbclark pushed a commit that referenced this pull request Sep 27, 2023
…VE (#10602)

* Fixes #36462 - check for presence of content facet
  before assigning CVE

(cherry picked from commit 2fdd82a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants