Stop serializing all hosts and template invocations in job invocation#1036
Merged
Conversation
Api::V2::JobInvocationsController#show endpoint serialized every host (via hosts/base) and every template invocation on each request, including the 1-second polling requests from the detail page. None of this data is used by the frontend — hosts are fetched separately by the host table, and per-host template invocations are fetched on row expand. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove dead @hosts assignment from create action - Extract @pattern_template_invocations into a before_action - Add test assertion for pattern_template_invocations presence Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The before_action was only used by the show action, so inline the assignment rather than extracting a separate method. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
Avoiding changing public API could save us from potential issues in the future. I would like to go with your suggested way to add a parameter. e.g. serialize_all_hosts (default: true). |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Author
I know, I know, it's just that it is something that should never have been added. Keeping compatibility is nice, having to opt-out from footguns not so much. Anyway, done, but if we go this way this will also need a corresponding change to h-rex to opt out from getting data it doesn't need. |
- Extract duplicated include_hosts? condition into a private method - Restore dropped 'should see only permitted hosts' authorization test - Add test for host_status=true with include_hosts=false (silent discard) - Add test for host_status=true with include_hosts=true (job_status present) - Merge redundant 'should include hosts by default' into 'should get invocation detail' Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
adamlazik1
approved these changes
Jul 1, 2026
adamlazik1
left a comment
adamlazik1
left a comment
Contributor
There was a problem hiding this comment.
LGTM, works as expected. Minor wording suggestion.
Co-authored-by: Adam Lazik <alazik@redhat.com>
adamlazik1
approved these changes
Jul 2, 2026
Contributor
|
Thanks @adamruzicka ! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
The biggest problem with this is that removing it changes a public api. To offer some alternatives, we could: