Ground work for implementation of CPE applicability language #7613
Conversation
vojtapolasek
added
Infrastructure
|
Skipping CI for Draft Pull Request. |
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
Hello @vojtapolasek! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:
Comment last updated at 2021-11-01 10:14:26 UTC |
vojtapolasek
force-pushed
the
cpe_al
branch
3 times, most recently
from
cee66f5
to
fedc742
Compare
ssg/build_cpe.py
Outdated
| initial_test.add_object(self.parse_platform_line(platform_line)) | ||
| return platform | ||
|
|
||
| def parse_platform_line(self, platform_line): |
There was a problem hiding this comment.
This function assumes that a platform_line has a single operator.
It should be possible to have platform_line like 'A & B & !C`.
There was a problem hiding this comment.
uhm... instead of trying to come up with a custom parser function... have you thought about building a simple recursive descent parser? Having a simple grammar definition might also be nice documentation for this.
I found a simple parser here: https://gist.github.com/iafisher/6f53ce4df29c91ac9276c13a113ccf9f
And the grammar (in kinda EBNF form) could look something like:
rule → CPE & rule
rule → !CPE
rule → CPE
Doing it this way would allow for building more complex entries such as the ones Yuuma is describing in his comment... And since these types of parsers are fairly common, it might even help in readability/maintenance as folks would know what to expect.
Adding to my previous comment about readable operands, using and and not as opposed to & and ! would also not incur much complexity, as they would be merely tokens in the parser which you could easily swap.
There was a problem hiding this comment.
BTW as you have already introduced the platform filter for profiles, I think that the CPE applicability may reuse that filter format.
|
The prefix |
|
Will this PR include docs on the expression format? If not, could you post it as part of this PR's description? That would help me review this as well. I'm hoping to take this into use as soon as it lands. |
|
Yes I plan to include documentation, I added it into the "what needs to be done" section. |
ssg/build_cpe.py
Outdated
| def _convert_platform_to_id(self, platform): | ||
| id = platform.replace(" ", "") | ||
| id = id.replace("&", "_and_") | ||
| id = id.replace("!", "not_") |
There was a problem hiding this comment.
instead of using C-style operands, why not use human-readable ones already? As we're already doing with filter_rules? That would provide some consistency and also would make this easier to read for non-programmers.
ssg/build_cpe.py
Outdated
| initial_test.add_object(self.parse_platform_line(platform_line)) | ||
| return platform | ||
|
|
||
| def parse_platform_line(self, platform_line): |
There was a problem hiding this comment.
uhm... instead of trying to come up with a custom parser function... have you thought about building a simple recursive descent parser? Having a simple grammar definition might also be nice documentation for this.
I found a simple parser here: https://gist.github.com/iafisher/6f53ce4df29c91ac9276c13a113ccf9f
And the grammar (in kinda EBNF form) could look something like:
rule → CPE & rule
rule → !CPE
rule → CPE
Doing it this way would allow for building more complex entries such as the ones Yuuma is describing in his comment... And since these types of parsers are fairly common, it might even help in readability/maintenance as folks would know what to expect.
Adding to my previous comment about readable operands, using and and not as opposed to & and ! would also not incur much complexity, as they would be merely tokens in the parser which you could easily swap.
vojtapolasek
changed the title
|
Thank you everyone for suggestions. The grammar parser and more human readable syntax sound really good. Due to recent changes and overall complexity of the change, I decided to steer this PR in a bit different direction. |
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
| ns = PREFIX_TO_NS[prefix] | ||
|
|
||
| def __init__(self): | ||
| self.platforms = [] |
There was a problem hiding this comment.
I think platforms could be a set().
There was a problem hiding this comment.
One issue that I see with this PR is that it introduces changes to the build system in form of code that is not used with the current content, and it is not tested either.
I have noticed commits that suggest that they "fix" something, but how would one noticed that something was broken?
I think that unit tests of newly introduced classes are easy to write and they would also define the intended behavior.
I think they are used, the Rule platforms now reference the CPE platforms
From what I understood most of the commits with "fix something" are addressing issues added by the PR itself.
I agree |
|
I will try to add unit tests as well. That is a good point. I think that the only introduced part of the code which is not used is the part which does some complex parsing of platform expressions. This will be changed in subsequent pull requests. So I don't plan to write tests for this part of the code. |
|
@vojtapolasek: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
they are not used now and they could cause confusion
replace cpe_al_* with cpe_*
substrings were not replaced
for profiles I am still waiting
rework comparing of platformspecifications, logicaltests and factrefs when dumping logicaltest into xml, nested logicaltests must go first and then factrefs
this step was not done in all situations and it caused datastream contain references to not defined platforms
it is not dependent on ProductCPE class
it is independent
it is independent
the logic might come back in future improvements
|
This PR introduces the SCAP-side support for more complex rule applicability by means of the CPE Applicability Language. I have proposed to the code that was originally present in this PR that bridged this new functionality with rule definitions. This is a problem that can be addressed separately, and that is, in itself, quite complex, as it has to appeal to content authors, and it has to be usable by remediations etc. Therefore, the current PR doesn't contain code that is suitable for unit-testing - the ability to express a complex platform definition in XML can be tested by means of system tests using a scanner, whereas the ability to extract a platform definition from a rule yaml into its intermediate representation will need unit tests. |
The machine platform applicability definition in rules/groups has changed, so the test suite had to be extended accordingly.
|
I am in favor of merging the PR. One set of suggestions became not applicable because the scope has been reduced, while others are still somehow relevant, but the discussion around them got silent, and they can be addressed subsequently. |
matejak
dismissed
JAORMX’s stale review
Suggestions became not applicable because the scope has been reduced. They will be relevant in another PRs.
Description: