(RHINENG-12028): serialize_host refactoring #2240
Merged
+74
−84
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
strider
force-pushed
the
RHINENG-12028-Serialize_host_refactoring
branch
5 times, most recently
from
89b4136 to
524217f
Compare
strider
force-pushed
the
RHINENG-12028-Serialize_host_refactoring
branch
4 times, most recently
from
3c5f7e1 to
daad157
Compare
jpramos123
requested changes
Feb 10, 2025
jpramos123
reviewed
Feb 10, 2025
|
I agree with @jpramos123's comments, but I think the code looks good beyond that. Good work! |
|
LGTM to except the moving of |
|
Thanks @thearifismail, @kruai, and @jpramos123 for your comments. I'll address that asap. |
strider
force-pushed
the
RHINENG-12028-Serialize_host_refactoring
branch
from
daad157 to
c752431
Compare
strider
changed the title
jpramos123
force-pushed
the
RHINENG-12028-Serialize_host_refactoring
branch
from
c752431 to
6220f6b
Compare
|
/retest |
jpramos123
approved these changes
Feb 12, 2025
strider
force-pushed
the
RHINENG-12028-Serialize_host_refactoring
branch
from
6220f6b to
700ebd7
Compare
Issues identified:
Repetitive staleness calculation:
* The stale_timestamp, stale_warning_timestamp, and culled_timestamp
assignments are redundant for edge and conventional hosts.
* We should extract this logic into a helper function.
Field processing in a long if-else chain:
* The current approach of checking each field separately makes it
harder to read.
* Using a dictionary comprehension may make it cleaner.
Unused arguments (additional_fields defaulted to tuple()):
* We should use None as a default, and convert it to a tuple inside
the function.
Better separation of concerns:
* Extract helper functions to make the serialize_host function more modular.
Improvements Proposal:
Encapsulating the staleness calculation:
* Extracting the calculation into a get_staleness_timestamps() helper
function to avoid duplicate logic.
Reducing repetitive "if" Checks:
* By using a dictionary field_mapping to map field names to their
corresponding serialization logic.
* By doing this, we eliminate long "if" chains, making it cleaner and
easier to modify.
Optimizing System Profile management:
* By using a dictionary comprehension to filter system_profile_facts
efficiently.
Improving readability and Maintainability:
* By bringing a better logical separation of concerns.
* By bringing an easier way to extend the host data (we just need to
modify the field_mapping and add/remove fields).
Signed-off-by: Gaël Chamoulaud <[email protected]>
rh-pre-commit.version: 2.3.2
rh-pre-commit.check-secrets: ENABLED
strider
force-pushed
the
RHINENG-12028-Serialize_host_refactoring
branch
from
700ebd7 to
7d4e3a7
Compare
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.
Overview
This PR is being created to address RHINENG-12028.
See my comment with all the explanations about this refactoring and improvements that can be done, in the Jira ticket above or in the commit message.
PR Checklist
Secure Coding Practices Documentation Reference
You can find documentation on this checklist here.
Secure Coding Checklist