Replace DataStreamHandler and BenchmarkHandler #147
Conversation
The new class SCAPContentHandler can handle SCAP source data streams, XCCDF files and tailoring files using common code. The classes DataStreamHandler and BenchmarkHandler are removed. The purpose of the new class is the same as of the removed classes. The purpose is to get a list of profiles (their IDs and descriptions) to be able to show it in the GUI. It is used only in the GUI.
|
Hello @jan-cerny! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-06-09 13:53:09 UTC |
|
Build finished. |
|
I'm thinking how to test it and I have already found multiple combinations that we need to test: a. scap-security-guide all of the situations need to be tested in both with and without kickstart These classes are used only in GUI so I think we don't need to test the text mode. |
Let users choose from profiles from both tailoring and the main SCAP content file. This is useful for content from an archive or a RPM file.
|
Build finished. |
| elif benchmark.tag.startswith(f"{{{ns['xccdf-1.2']}}}"): | ||
| xccdf_ns_prefix = "xccdf-1.2" | ||
| else: | ||
| raise SCAPContentHandlerError("Unsupported XML namespace") |
There was a problem hiding this comment.
Would be nice to indicate what namespace is unsupported.
| self._data_stream_id = data_stream_id | ||
| self._checklist_id = checklist_id | ||
|
|
||
| def get_profiles(self): |
There was a problem hiding this comment.
Those nested ifs are confusing. I propose to restructure it in the following way:
Firs identify all pathological cases, throw all the easy errors - bad XCCDF, standalone tailoring or unknown content.
Then, there is the trivial benchmark extraction from XCCDF, and multi-step benchmark extraction from a datastream, that I propose to extract to another method.
| self.file_path = file_path | ||
| tree = ET.parse(file_path) | ||
| self.root = tree.getroot() | ||
| if tailoring_file_path is not None: |
There was a problem hiding this comment.
You can reduce to a Pythonic if not tailoring_file_path, which would handle empty strings as well, and you could therefore streamline some of the higher-level code.
org_fedora_oscap/gui/spokes/oscap.py
Outdated
| if self._addon_data.preinst_tailoring_path: | ||
| tailoring_path = self._addon_data.preinst_tailoring_path | ||
| else: | ||
| tailoring_path = None |
There was a problem hiding this comment.
This can be streamlined - see another comment below.
tests/test_scap_content_handler.py
Outdated
| title="Default", | ||
| description="The implicit XCCDF profile. Usually, the default profile " | ||
| "contains no rules.") | ||
| assert pinfo1 in profiles |
There was a problem hiding this comment.
Please add a newline, text files that don't end by a newline are considered dubious.
org_fedora_oscap/gui/spokes/oscap.py
Outdated
| else: | ||
| # pylint: disable-msg=E1103 | ||
| profiles = self._content_handler.profiles | ||
| profiles = self._content_handler.get_profiles() |
There was a problem hiding this comment.
This can lead to exceptions, so what about dispatching a logging message that something went wrong with the update of profiles, and pass the exception on?
The previous behavior was equally bad about this, and any exception would probably kill the addon, so I wouldn't concentrate on this aspect in this PR.
This way will allow us to emit e more descriptive error message.
The code that extracts the pointer to root element of an XCCDF Benchmark has been extracted out to a new private method `_parse_profiles_from_xccdf`. Also, the conditions in get_profiles() have been reorganized so that the function now checks for all possible errors and proceeds only when the checks succeed.
|
Build finished. |
Addressing: The variable benchmark does not seem to be defined for all execution paths.
|
Build finished. |
|
The inspection completed: 23 updated code elements |
|
The handler works as expected, and it is also much faster than the previous implementation. Great work, thanks! |
The new class SCAPContentHandler can handle SCAP source data
streams, XCCDF files and tailoring files using common code.
The classes DataStreamHandler and BenchmarkHandler are removed.
The purpose of the new class is the same as of the removed
classes. The purpose is to get a list of profiles (their IDs
and descriptions) to be able to show it in the GUI. It is
used only in the GUI.