-
Notifications
You must be signed in to change notification settings - Fork 685
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Platforms can accept parameters and pass them to underlying CPE items #9799
Platforms can accept parameters and pass them to underlying CPE items #9799
Conversation
add a function which uses the built-in string formatting function to replace parts denoted by {} by strings supplied as a dictionary
…store them it is now possible to specify expressions like package[ntp] and the CPEALFActref will receive the proper parameter which is used further. the pkg_resources module is used to parse the parameter correctly.
Now, when creating a platform, you can specify for example package[ntp] This will search for CPE item called "package", ignoring the part in brackets. The item MUST have the "name" attribute parametrized, so that it can get a new name. After it finds it, it will load it and create a new item with content where {arg} is replaced by the parameter given to the platform.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's amazing. I like that you add the unit tests.
I think you don't need to fix the congitive complexity of apply_formatting_on_dict_values that is reported by CodeClimate. I think that the function is isolated enough and readable.
I have 2 small suggestions below.
ssg/boolean_expression.py
Outdated
@@ -73,7 +73,10 @@ def __init__(self, obj): | |||
self.obj = self.spec | |||
|
|||
def __call__(self, **kwargs): | |||
val = kwargs.get(self.name, False) | |||
full_name = self.name | |||
if self.spec.extras: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's confusing what self.spec.extras
is and what it contains.
I have found that it's from pkg_resources Requirement class, but it took me some time.
It would be better to somehow rename it or at least add comments explaining it.
I have a quick idea which I admit that I haven't tried but you can try it:
- add a comment to the definition of the
arg
property, 2. - the
arg
property would be the only place whereself.spec.extras
are used - everywhere else where you use the
self.spec.extras
you can use thearg
property instead.
ssg/build_cpe.py
Outdated
@@ -87,6 +88,9 @@ def get_cpe(self, ref): | |||
if self._is_name(ref): | |||
return self.cpes_by_name[ref] | |||
else: | |||
# in case we get parametrized item in form ref[parameter] | |||
# get only the ref part | |||
ref = pkg_resources.Requirement.parse(ref).project_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think It would be better to add some more abstraction here and "hide" the Requirement class. You can extract this line to a new separate function with a good name.
use Symbol.arg when possible rename Symbol.spec to Symbol-requirement rename function specs to version_definitions update unit test
@jan-cerny I think I addressed your feedback. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vojtapolasek mostly looks great but unfortunately new codeclimate issues appeared now.
The Fedora Rawhide fail is in oscap xccdf generate fix
and therefore isn't related to this PR.
… platform id if yes, extract only the base name
b4c9dfd
to
175441a
Compare
Code Climate has analyzed commit 175441a and detected 1 issue on this pull request. Here's the issue category breakdown:
The test coverage on the diff in this pull request is 100.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 47.2% (0.4% change). View more on Code Climate. |
@jan-cerny I fixed it. I melded the fix into the last commit. I hope it is ok. |
def enum(*args): | ||
enums = dict(zip(args, range(len(args)))) | ||
return type('Enum', (), enums) | ||
|
||
|
||
def apply_formatting_on_dict_values(source_dict, string_dict, ignored_keys=[]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Passing a mutable object as a default argument is potentially dangerous - it is a global object, so if the function adds anything to it during the execution, then subsequent calls won't have a default of an empty list. For that reason, it is common to have default values e.g. None
and to have statements s.a. ignored_keys = ignored_keys or []
in the function.
And second of all, this function could be split very nicely into a part that handles the outer loop and ignored keys, and another part that handles the processing of individual values with a poetic name s.a. recurse_or_substitute_or_do_nothing
.
|
||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why these whitespaces weren't caught by our checks?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Answer to myself: Because we exempt tests from style checks.
@@ -367,6 +367,17 @@ def test_platform_get_invalid_conditional_language(product_cpes): | |||
with pytest.raises(AttributeError): | |||
assert platform.get_remediation_conditional("foo") | |||
|
|||
def test_parametrized_platform(product_cpes): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great test, indeed looks as a specification.
@@ -257,10 +269,20 @@ def enrich_with_cpe_info(self, cpe_products): | |||
self.ansible_conditional = cpe_products.get_cpe(self.cpe_name).ansible_conditional | |||
self.cpe_name = cpe_products.get_cpe_name(self.cpe_name) | |||
|
|||
def pass_parameters(self, product_cpes): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is pretty difficult to deduce what the method does from its name. And it is not easy to deduce that even from the body. Do we pass parameters to self, or to product CPEs? What parameters do we pass? The one that we call arg
, or is it about something else? Such questions arise, and they are not easily answered by looking at the code.
@@ -82,11 +84,16 @@ def add_cpe_item(self, cpe_item): | |||
def _is_name(self, ref): | |||
return ref.startswith("cpe:") | |||
|
|||
def _is_parametrized(self, ref): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bad place for this method. It is a private method, we don't know that could possibly "ref" be, and we don't use self
inside that method.
Most importantly, the class that knows about the syntax of parametrized platform units is the class that parses those definition strings, so it should be a static method of Symbol.
@@ -198,6 +206,10 @@ def enrich_with_cpe_info(self, cpe_products): | |||
for arg in self.args: | |||
arg.enrich_with_cpe_info(cpe_products) | |||
|
|||
def pass_parameters(self, product_cpes): | |||
for arg in self.args: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have args
and arg
here, but just one level below (but on the same level of abstraction), arg
means something completely else. This is a sign of bad naming, but probably arg
is a bad name for both cases - it is too generic.
|
||
def __call__(self, **kwargs): | ||
val = kwargs.get(self.name, False) | ||
if len(self.spec.specs): | ||
full_name = self.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code is mixes a lot of abstraction levels together.
It is not clear why the object representing a parsed platform unit specification should be callable in the first place, and the underlying code is convoluted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code know nothing about platforms, CPEs etc. It is an expansion of boolean.py's classes Function and Symbol which understand and can operate on boolean expressions that contain pkg_resources.Requirement as individual elements. The __init__
, __call__
and __lt__
overload is essential for those symbols to be correctly compared, sorted and optimized, this is how boolean.py operates.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it then be possible to represent the parser and parsed results as different classes? The current approach violates the interface segregation principle, and implementation bits of parsers are leaked to the code that is only interested in trees or platform units.
|
||
@property | ||
def specs(self): | ||
return self.spec.specs | ||
def version_definitions(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This property isn't mentioned in the PR description, and not tested either. However, it is used in quite a complex way in the new code.
Looking at other places, I would probably skip this method, and go for a method that converts underlying version specifiers to strings, because this is what this property is used for ATM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should not be there at all. We are not introducing versions ATM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I would suggest to remove it in as subsequent PR, or to change the approach - the code used in as_id
and in PlatformSymbol.as_cpe_lang_xml
is both similar to each other, and both places depend on a particular representation of the version spec.
Description:
This PR adds a mechanism which allows platforms to be specified as
If such a platform is encountered, the following happens:
If the platform does not have parameter in [], no change to the processing should happen.
Note that the functionality is not used in the content. But it is demonstrated in unit tests.
See commit messages as well.
Rationale:
This is aprt of an initiative which should allow content authors to pass parameters to platform definitions, therefore creating multiple platforms with common base attributes but with slight deviations.