grains: Split os_data into smaller functions #61597
Merged
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
bdrung
force-pushed
the
split_os_grain
branch
2 times, most recently
from
bd7eea1
to
3711114
Compare
bdrung
changed the title
bdrung
force-pushed
the
split_os_grain
branch
3 times, most recently
from
1f9d621
to
61b9d1e
Compare
|
Rebased and added two more commits. |
bdrung
force-pushed
the
split_os_grain
branch
2 times, most recently
from
615eaee
to
75fcf63
Compare
bdrung
force-pushed
the
split_os_grain
branch
from
75fcf63
to
a98dbe8
Compare
bdrung
force-pushed
the
split_os_grain
branch
from
a98dbe8
to
9d278d6
Compare
|
Rebased and added a test case for |
bdrung
force-pushed
the
split_os_grain
branch
from
9d278d6
to
a4fcc05
Compare
The `os_data` function is huge (531 lines for one function is too long) and also deeply nested (8 levels deep). That make reading, modifying, and testing this function harder. Split `os_data` into smaller functions to a readable 167 lines with only a nesting level of three. Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
`os_data` has a case-like statement to differentiate the different operating systems, but this is split into two parts. To make the code more readable, combine all case-like statement in `os_data` to use one long if-elif chain. Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
Move fetching `lsb_distrib_*` grains into separate function `_linux_lsb_distrib_data` to make `_linux_distribution_data` shorter. Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
To make the code easier to read, set `os_family` in each branch in `os_data()`. Most branches have an hard-coded OS family and can be dropped from `_OS_FAMILY_MAP`. The dictionary `_OS_FAMILY_MAP` is now only used for deriving the OS family of Linux. Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
In `os_data()` only the kernel branch for `SunOS` might not set the `os` grain. To make the code easier to read, ensure that `os` is set in each branch in `os_data()`. Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
The test cases for `os_data` do not cover the case that `lsb_release` is imported successfully. So add a test case for that. Signed-off-by: Benjamin Drung <benjamin.drung@ionos.com>
bdrung
force-pushed
the
split_os_grain
branch
from
a4fcc05
to
5344c7b
Compare
|
@dmurphy18 #61626 is the final merge request containing all changes. Since that merge request is big, I split it into multiple smaller chunks (that stack on each other). This merge request is the first one. I rebased it on master and resolved the conflict. |
dmurphy18
previously approved these changes
Sep 23, 2022
dmurphy18
dismissed
their stale review
Forgot still needs a changelog entry
dmurphy18
requested changes
Sep 23, 2022
|
Why does this pull request need a changelog entry? It does not change anything for the user. Only #61626 has user facing changes and changelog entries. |
|
@bdrung Sorry, so used to everything needing a changelog. You are correct, if it did not impact user and was only internal things it does not add a changelog, approving this |
dmurphy18
approved these changes
Sep 26, 2022
|
Thanks. The next pull request in this series is #61589 |
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.
The
os_datafunction is huge (531 lines for one function is too long) and also deeply nested (8 levels deep). That make reading, modifying, and testing this function harder.Split
os_datainto smaller functions to a readable 167 lines with only a nesting level of three.os_datahas a case-like statement to differentiate the different operating systems, but this is split into two parts.To make the code more readable, combine all case-like statement in
os_datato use one long if-elif chain.