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 #37178 - use safe navigation for operatingsystem #10897

Merged
merged 1 commit into from
Feb 28, 2024

Conversation

jeremylenz
Copy link
Member

What are the changes introduced in this pull request?

Use Ruby safe navigation operator for operatingsystem. This fixes a bug introduced in #10881

Considerations taken when implementing this change?

What are the testing steps for this pull request?

Clone a registered rhel8 host
Make sure you can display the host details page without getting a 500

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I think unmanaged systems don't have an OS, so this is correct.

In hindsight we should have nested the whole OS as its own node, rather than a set of properties. I can see this is consistent with operatingsystem_id and operatingsystem_name`, but this would have been better IMHO:

{
  "operatingsystem": {
    "id": 1,
    "name": "",
    "family": "",
    "major": ""
  }
}

But that would mean a major redesign of the API and I don't see that happening soon.

Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

Code change looks fine and going off what Ewoud said, this looks ok. Going to start testing

@chris1984 chris1984 self-assigned this Feb 26, 2024
Copy link
Member

@chris1984 chris1984 left a comment

Choose a reason for hiding this comment

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

ACK,

Looks good, before PR:

14:02:26 rails.1   | 2024-02-27T14:02:26 [I|app|8bb69bda] Backtrace for 'Action failed' error (ActionView::Template::Error): undefined method `family' for nil:NilClass
14:02:26 rails.1   |  8bb69bda | /home/vagrant/katello/app/views/katello/api/v2/hosts/base.json.rabl:10:in `block in cached_source_4461041918095890411'

prbefore

After PR:
No error
prafter

@jeremylenz jeremylenz force-pushed the 37178-nilclass-os-family branch from 75fe5a5 to b755f59 Compare February 27, 2024 19:21
@jeremylenz
Copy link
Member Author

rebased

@ianballou
Copy link
Member

[test katello]

@jeremylenz jeremylenz merged commit 664488d into Katello:master Feb 28, 2024
14 of 16 checks passed
sbernhard pushed a commit to ATIX-AG/katello that referenced this pull request Feb 29, 2024
qcjames53 pushed a commit to qcjames53/katello that referenced this pull request Mar 7, 2024
qcjames53 added a commit that referenced this pull request Mar 7, 2024
* Release 4.12.0-rc1 (#10909)

* Fixes #37137 - Use insert_all when creating available module streams (#10878)

* Fixes #37137 - Use insert_all when creating available module streams

(cherry picked from commit 7458f70)

* Fixes #37178 - use safe navigation for operatingsystem (#10897)

(cherry picked from commit 664488d)

* Refs #37202 - display name according to setting in host collection

(cherry picked from commit de39a36)

* Fixes #37192 - Use with_enabled_email scope (#10901)

* Fixes #37192 - Use with_enabled_email scope

Instead of typing out the same query explicitly.

* Refs #37192 - Bump required foreman version

(cherry picked from commit 1ace8e5)

* Refs #37203 - display name according to setting in errata host list

(cherry picked from commit 2775554)

* Fixes #37214 - Change 'default' for limit to environment checkbox in activation-key and content-host

(cherry picked from commit 9d0a338)

* Fixes #37108 - Preload content_view_components (#10864)

(cherry picked from commit d98c17b)

* Fixes #36976 - Too many audit records slow down CV loading (#10911)

(cherry picked from commit 30c6977)

* Refs #37201 - display short host name in activation keys menu

(cherry picked from commit 8164043)

* Fixes #37187 - Update ACS refresh Pulp fixtures + fix repository_test test

(cherry picked from commit 99c7857)

* Fixes #37198 - Allow installedDeb package attributes in safemode

(cherry picked from commit b5d785f)

* Fixes #37197 - Kickstart repository correctly listed on hostgroup

(cherry picked from commit 35c49b1)

* Fixes #37169 - Managing a Hosts Repository Sets does not behave as expected (#10905)

(cherry picked from commit 6420cf8)

---------

Co-authored-by: Jeremy Lenz <[email protected]>
Co-authored-by: Matěj Mudra <[email protected]>
Co-authored-by: Adam Růžička <[email protected]>
Co-authored-by: Manisha Singhal <[email protected]>
Co-authored-by: Bernhard Suttner <[email protected]>
Co-authored-by: Samir Jha <[email protected]>
Co-authored-by: ianballou <[email protected]>
Co-authored-by: Bernhard Suttner <[email protected]>
Co-authored-by: Partha Aji <[email protected]>
Co-authored-by: Thorben <[email protected]>
sbernhard pushed a commit to ATIX-AG/katello that referenced this pull request Mar 8, 2024
qcjames53 pushed a commit to qcjames53/katello that referenced this pull request Mar 8, 2024
qcjames53 added a commit that referenced this pull request Mar 8, 2024
* Fixes #37187 - Update ACS refresh Pulp fixtures + fix repository_test test

(cherry picked from commit 99c7857)

* Fixes #37137 - Use insert_all when creating available module streams (#10878)

* Fixes #37137 - Use insert_all when creating available module streams

(cherry picked from commit 7458f70)

* Fixes #37178 - use safe navigation for operatingsystem (#10897)

(cherry picked from commit 664488d)

* Refs #37202 - display name according to setting in host collection

(cherry picked from commit de39a36)

* Refs #37201 - display short host name in activation keys menu

(cherry picked from commit 8164043)

* Refs #37203 - display name according to setting in errata host list

(cherry picked from commit 2775554)

* Fixes #37214 - Change 'default' for limit to environment checkbox in activation-key and content-host

(cherry picked from commit 9d0a338)

* Fixes #37198 - Allow installedDeb package attributes in safemode

(cherry picked from commit b5d785f)

* Fixes #37108 - Preload content_view_components (#10864)

(cherry picked from commit d98c17b)

* Fixes #37197 - Kickstart repository correctly listed on hostgroup

(cherry picked from commit 35c49b1)

* Fixes #36976 - Too many audit records slow down CV loading (#10911)

(cherry picked from commit 30c6977)

* Fixes #37169 - Managing a Hosts Repository Sets does not behave as expected (#10905)

(cherry picked from commit 6420cf8)

---------

Co-authored-by: ianballou <[email protected]>
Co-authored-by: Jeremy Lenz <[email protected]>
Co-authored-by: Matěj Mudra <[email protected]>
Co-authored-by: Manisha Singhal <[email protected]>
Co-authored-by: Bernhard Suttner <[email protected]>
Co-authored-by: Bernhard Suttner <[email protected]>
Co-authored-by: Partha Aji <[email protected]>
Co-authored-by: Samir Jha <[email protected]>
Co-authored-by: Thorben <[email protected]>
qcjames53 pushed a commit to qcjames53/katello that referenced this pull request Mar 8, 2024
qcjames53 added a commit that referenced this pull request Mar 8, 2024
* Fixes #37187 - Update ACS refresh Pulp fixtures + fix repository_test test

(cherry picked from commit 99c7857)

* Fixes #37137 - Use insert_all when creating available module streams (#10878)

* Fixes #37137 - Use insert_all when creating available module streams

(cherry picked from commit 7458f70)

* Fixes #37178 - use safe navigation for operatingsystem (#10897)

(cherry picked from commit 664488d)

* Refs #37202 - display name according to setting in host collection

(cherry picked from commit de39a36)

* Refs #37201 - display short host name in activation keys menu

(cherry picked from commit 8164043)

* Refs #37203 - display name according to setting in errata host list

(cherry picked from commit 2775554)

* Fixes #37214 - Change 'default' for limit to environment checkbox in activation-key and content-host

(cherry picked from commit 9d0a338)

* Fixes #37198 - Allow installedDeb package attributes in safemode

(cherry picked from commit b5d785f)

* Fixes #37108 - Preload content_view_components (#10864)

(cherry picked from commit d98c17b)

* Fixes #37197 - Kickstart repository correctly listed on hostgroup

(cherry picked from commit 35c49b1)

* Fixes #36976 - Too many audit records slow down CV loading (#10911)

(cherry picked from commit 30c6977)

* Fixes #37169 - Managing a Hosts Repository Sets does not behave as expected (#10905)

(cherry picked from commit 6420cf8)

* Refs #37214 - Use Limit to Environment by default

(cherry picked from commit 8bedda9)

---------

Co-authored-by: ianballou <[email protected]>
Co-authored-by: Jeremy Lenz <[email protected]>
Co-authored-by: Matěj Mudra <[email protected]>
Co-authored-by: Manisha Singhal <[email protected]>
Co-authored-by: Bernhard Suttner <[email protected]>
Co-authored-by: Bernhard Suttner <[email protected]>
Co-authored-by: Partha Aji <[email protected]>
Co-authored-by: Samir Jha <[email protected]>
Co-authored-by: Thorben <[email protected]>
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