-
Notifications
You must be signed in to change notification settings - Fork 284
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 #36440 - Redefine #attributes= to hijack old content facet attributes #10600
Merged
jeremylenz
merged 2 commits into
Katello:master
from
jeremylenz:36440-wtf-attributes-equals
Jun 16, 2023
Merged
Fixes #36440 - Redefine #attributes= to hijack old content facet attributes #10600
jeremylenz
merged 2 commits into
Katello:master
from
jeremylenz:36440-wtf-attributes-equals
Jun 16, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Issues: #36440 |
ianballou
reviewed
Jun 16, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Working great. Maybe add a quick test that runs attributes= on a host?
@ianballou Updated 👍 |
ianballou
approved these changes
Jun 16, 2023
lfu
pushed a commit
to lfu/katello
that referenced
this pull request
Jun 21, 2023
…ibutes (Katello#10600) * Fixes #36440 - Redefine #attributes= to hijack old content facet attributes * Refs #36440 - add tests (cherry picked from commit 0a20051)
wbclark
pushed a commit
to wbclark/katello
that referenced
this pull request
Sep 7, 2023
…ibutes (Katello#10600) * Fixes #36440 - Redefine #attributes= to hijack old content facet attributes * Refs #36440 - add tests (cherry picked from commit 0a20051)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are the changes introduced in this pull request?
When updating a host using the
Api::V2::HostController
, Foreman doesn't call@host.update
- instead, it doesThe consequence of this is that the host's
update
method redefined inhost_managed_extensions
was never getting called. This is what removed the old deprecated content facet attributescontent_view_id
andlifecycle_environment_id
, so as a consequence, those attributes were still present when Foreman called@host.attributes=
. Turns out the#attributes=
method checks and validates all the nested attributes, so when we try to include imaginary ones likecontent_view_id
, it would throw an error:With this change, the
attributes=
method ofHost::Managed
gets the same enhancement thatupdate
already had. This will hopefully prevent this problem until we can properly finish the multi-CV related work.Considerations taken when implementing this change?
Just like #10595, this issue should go away completely when we update
Hostgroup::ContentFacet
to truly handle multiple content views.What are the testing steps for this pull request?
Create a hostgroup and assign both CV and LCE
Assign a host to it
Update the host via hammer (this is important because hammer will use the API hosts controller, not the regular one) and include a hostgroup update:
Before: you should get the error above in the Rails log, and Hammer will complain:
After: