feat: Move test formatter from .github/workflows/format_diff.py to python-gardenlinux-lib#294
feat: Move test formatter from .github/workflows/format_diff.py to python-gardenlinux-lib#294
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
==========================================
- Coverage 91.55% 91.26% -0.29%
==========================================
Files 42 46 +4
Lines 2083 2394 +311
==========================================
+ Hits 1907 2185 +278
- Misses 176 209 +33 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| commit: str | ||
|
|
||
|
|
||
| class Formatter(object): |
There was a problem hiding this comment.
File name and class name should be the same. Furthermore the class name should indicate what it formats or into which format. I guess MarkdownFormatter might be most suitable as we do not intend to support multiple output formats anyway.
To not loose what it formats a package called gardenlinux.features.reproducibility might be suitable
There was a problem hiding this comment.
This file mixes output and parsing logic. Separating the functionality might allow reusability.
| diff_dir = pathlib.Path(gardenlinux_root).joinpath(diff_dir) | ||
|
|
||
| self._all = set() | ||
| self._flavors = os.listdir(diff_dir) |
There was a problem hiding this comment.
This variable indicates a different content then what it contains. Based on GardenLinux naming convention a flavor is a specific set of features as a string. E.g. gcp-gardener_prod_trustedboot resulting in a GardenLinux canonical name if appended with the CPU architecture, version and commit.
This variable contains files produced during one build if I understand it correctly.
|
|
||
| remove_arch = re.compile("(-arm64|-amd64)$") | ||
|
|
||
| def __init__( |
There was a problem hiding this comment.
The constructor does way too much. Usually a constructor should only set-up an instance. If needed it might call class methods to increase readability.
|
|
||
| self._parser = Parser(gardenlinux_root, feature_dir_name, logger) | ||
| if gardenlinux_root is None: | ||
| gardenlinux_root = self._parser._GARDENLINUX_ROOT |
There was a problem hiding this comment.
Accessing private members are discouraged. If needed Parser can be extended to provide the directory information used at an instance of it.
| :since: 1.0.0 | ||
| """ | ||
|
|
||
| if len(items) <= 10: |
There was a problem hiding this comment.
Magic numbers - please use a constant for the limit :)
| found.update(fnd) | ||
| s += " " + st.replace("\n", "\n ") + "\n" | ||
| # Remove last linebreak as the last line can contain spaces | ||
| return "\n".join(s.split("\n")[:-1]), found |
There was a problem hiding this comment.
Wouldn't rstrip() be more appropriate here?
| first = True | ||
| tree = None | ||
| for flavor in flavors: | ||
| if not flavor.startswith("bare-"): |
There was a problem hiding this comment.
Hidden magic is always a request for hard to debug issues later on. Please add # @TODO comments where applicable.
| """ | ||
|
|
||
| with open( | ||
| self._parser._feature_base_dir.joinpath(f"{node}/info.yaml"), "r" |
There was a problem hiding this comment.
You already accessed the private root directory variable from Parser. Best practice (if not otherwise possible) would be to store it in this instance in __init__() for later reuse.
There was a problem hiding this comment.
I would expect gl-diff to at least additionally produce the diff files formatted here as well. Based on if that would be feasible the file name should either indicate it's handling flavor build differential content or it's only parsing and providing output for it.
|
Thanks for your detailed feedback. I have heavily restricted the formatter and implemented all your comments. It's still a massive PR, as it also contains the difference generator now. So take your time! I was not able to find a python package or better way to unpack .oci containers, so I've implemented unpacking logic myself. It works so far. |
2 participants
What this PR does / why we need it:
workflowsdirectory, to use features of the library and to be able to test the output with simple unit tests