-
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
Refactoring: Remove all references to prodtype (code/tests/docs) #11505
Refactoring: Remove all references to prodtype (code/tests/docs) #11505
Conversation
Skipping CI for Draft Pull Request. |
2bcb1bf
to
380644e
Compare
This datastream diff is auto generated by the check Click here to see the full diffNew content has different text for rule 'xccdf_org.ssgproject.content_rule_package_audit-audispd-plugins_installed'.
--- xccdf_org.ssgproject.content_rule_package_audit-audispd-plugins_installed
+++ xccdf_org.ssgproject.content_rule_package_audit-audispd-plugins_installed
@@ -4,6 +4,9 @@
[description]:
The audit-audispd-plugins package should be installed.
+
+[warning]:
+This package is not available in Red Hat Enterprise Linux 8 [rhel8]. The correct package is called audispd-plugins.
[reference]:
CCI-001851
New content has different text for rule 'xccdf_org.ssgproject.content_rule_service_ntp_enabled'.
--- xccdf_org.ssgproject.content_rule_service_ntp_enabled
+++ xccdf_org.ssgproject.content_rule_service_ntp_enabled
@@ -5,6 +5,9 @@
[description]:
The ntp service can be enabled with the following command:
$ sudo systemctl enable ntp.service
+
+[warning]:
+The ntp package is not available in Red Hat Enterprise Linux 8. Please consider the chrony package instead together with the respective service_chronyd_enabled rule.
[reference]:
1
New content has different text for rule 'xccdf_org.ssgproject.content_rule_service_ntpd_enabled'.
--- xccdf_org.ssgproject.content_rule_service_ntpd_enabled
+++ xccdf_org.ssgproject.content_rule_service_ntpd_enabled
@@ -5,6 +5,9 @@
[description]:
The ntpd service can be enabled with the following command:
$ sudo systemctl enable ntpd.service
+
+[warning]:
+The ntp package is not available in Red Hat Enterprise Linux 8. Please consider the chrony package instead together with the respective service_chronyd_enabled rule.
[reference]:
1 |
c20976d
to
5ea8d4c
Compare
5ea8d4c
to
a623596
Compare
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.
Overall, looks good.
Just more minor change that I'm not willing to block over.
tests/ssg_test_suite/common.py
Outdated
# Our local copy of env_yaml needs some properties from rule.yml | ||
# for completeness. | ||
local_env_yaml = dict() | ||
local_env_yaml.update(env_yaml) | ||
local_env_yaml['rule_id'] = rule.id_ | ||
local_env_yaml['rule_title'] = rule.title | ||
local_env_yaml['products'] = prodtypes | ||
local_env_yaml["products"] = {product} |
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.
local_env_yaml["products"] = {product} | |
local_env_yaml['products'] = {product} |
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.
Fixed.
@@ -21,7 +21,7 @@ description: |- | |||
rationale: |- | |||
These settings configure the firewall to allow connections to an FTP server. | |||
|
|||
{{% if prodtype != "rhel7" %}} | |||
{{% if product != "rhel7" %}} |
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.
There never was a Jinja variable named prodtype
. It doesn't make sense.
Also, it might be that the author (@cipherboy) actually meant to do the opposite. I'm not sure.
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.
\o hello @evgenyz -- Good question! It has been so long, I do not know. Likely I made a mistake and expected the build system to catch it...
warnings: | ||
- general: | ||
{{% if prodtype == "rhel7" %}} | ||
{{% if product == "rhel7" %}} |
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.
There never was a Jinja variable named prodtype
. It doesn't make sense.
@@ -50,7 +50,7 @@ template: | |||
|
|||
platform: package[ntp] | |||
|
|||
{{% if prodtype in ["rhel8", "rhel9", "sle15"] %}} | |||
{{% if product in ["rhel8", "rhel9", "sle15"] %}} |
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.
There never was a Jinja variable named prodtype
. It doesn't make sense.
@@ -34,9 +34,9 @@ template: | |||
pkgname@ubuntu1804: audispd-plugins | |||
pkgname@ubuntu2004: audispd-plugins | |||
|
|||
{{% if prodtype in ["rhel7", "rhel8", "rhel9"] %}} | |||
{{% if product in ["rhel7", "rhel8", "rhel9"] %}} |
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.
There never was a Jinja variable named prodtype
. It doesn't make sense.
@@ -7,7 +7,7 @@ Generates the :code:`<affected>` tag for OVAL check using correct product platfo | |||
#}} | |||
{{%- macro oval_affected(products) %}} | |||
<affected family="unix"> | |||
{{{ prodtype_to_platform(products)|indent(2) }}} | |||
{{{ product_to_platform(products)|indent(2) }}} |
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 was renamed to better reflect the intention. The product
we use in our code is usually the product slug.
|
||
|
||
def parse_platform(fix_contents): | ||
def find_platform_line(fix_contents): |
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.
Better reflects what it actually does.
|
||
return set(map(lambda x: x.strip(), platform.split(','))) | ||
|
||
|
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 has been transferred from the rule_yaml
module and renamed to reflect the scope of this new-old function, that is now used only to parse platforms.
Could you please check the 3-rd commit and maybe provide some insight about the change: is it going to do a good thing or a bad thing? |
They don't look actively harmful. |
If a rule needs to be chosen only in some of products despite its `prodtype` we | ||
can use Jinja macros inside the controls file to choose products. |
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.
Here I would remove only "despite its prodtype".You can still use Jinja macros inside controls file to choose products in which the rule should appear. For example, you can add this to a control file:
{{% if product == "rhel9" %}}
- partition_for_tmp
{{% endif %}}
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.
Fixed.
fields in the content itself (e.g., if `rhel7` is not present in the | ||
`rhel7.xml` OVAL check platform specifier, it will be included in the | ||
content will not override the contents of `platform` field in | ||
the content itself (e.g., if `rhel7` is not present in the `rhel7.xml` |
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 idea for work in future
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.
Yep. I cleaned up connections between prodtype and platforms, but kept them out of the scope. It is a zoo there.
1dc8a6c
to
08d3fb7
Compare
Code Climate has analyzed commit 08d3fb7 and detected 0 issues on this pull request. The test coverage on the diff in this pull request is 65.0% (50% is the threshold). This pull request will bring the total coverage in the repository to 58.3% (-0.1% change). View more on Code Climate. |
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 have reviewed code. I also run Automatus locally.
Description:
Pretty much what it says on the tin. After merging #11378 we don't have any use for
prodtype
as it always equals all.This PR removes (and occasionally renames) build system code, tools and documentation related to
prodtype
.Rationale:
Review Hints:
Closes: #11382.