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 #37043 - Inheritance overrides host facets #9990

Merged
merged 1 commit into from Mar 11, 2024

Conversation

ananace
Copy link
Member

@ananace ananace commented Jan 11, 2024

No description provided.

@jeremylenz
Copy link
Contributor

Will this affect the behavior of Katello/katello#10841 ?

cc @parthaa @qcjames53

@qcjames53
Copy link
Contributor

I'm not entirely sure to be honest. My hunch is we should be ok since I'm removing the apply_inherited_attributes method from Katello entirely. Once I finish up the new unit tests I'll run this PR through to see if anything breaks.

@ananace
Copy link
Member Author

ananace commented Feb 5, 2024

@ShimShtein

Copy link
Member

@ShimShtein ShimShtein left a comment

Choose a reason for hiding this comment

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

Left a comment to fix the explicit nil use case.

Also can we add some tests to https://github.com/theforeman/foreman/blob/develop/test/unit/facet_test.rb to specify this behavior?

app/models/concerns/facets/base.rb Outdated Show resolved Hide resolved
@ananace
Copy link
Member Author

ananace commented Feb 14, 2024

Redid merge code and added tests for both overriding with an actual value as well as with nil.

I don't know if the merge result will be exactly as expected with every potential mix of strings/symbols, but I wasn't able to make it misbehave in the tests, so I decided to skip slapping with_indifferent_access on everything.

@ShimShtein
Copy link
Member

@qcjames53 were you able to finish the unit tests? I would like to run this PR there just to be sure I am not missing some use case.

Otherwise LGTM.

@qcjames53
Copy link
Contributor

Hey @ShimShtein. The changes are merged into master branch Katello; you should be all good to go.

@jeremylenz
Copy link
Contributor

[test katello]

@ShimShtein ShimShtein merged commit 3a55f46 into theforeman:develop Mar 11, 2024
23 of 25 checks passed
@ShimShtein
Copy link
Member

Merged, thanks @ananace !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants