feat: Add parser and spec for compliance_policies_enabled #4498
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4498 +/- ##
==========================================
+ Coverage 77.64% 77.68% +0.03%
==========================================
Files 745 746 +1
Lines 41693 41730 +37
Branches 6703 6709 +6
==========================================
+ Hits 32373 32417 +44
+ Misses 8290 8284 -6
+ Partials 1030 1029 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
wushiqinlou
force-pushed
the
Add_parser_compliance_policies_enabled
branch
2 times, most recently
from
aaa4497 to
b727f14
Compare
wushiqinlou
changed the title
xiangce
added
data collection
|
@sourcery-ai title |
|
@sourcery-ai review |
sourcery-ai
bot
changed the title
Reviewer's GuideThis PR introduces a new datasource to collect enabled compliance policies and optional tailoring rules, refactors existing datasources to conditionally skip when compliance flags are absent, registers the new datasource and corresponding parser in the Specs, and adds comprehensive tests and documentation. ER diagram for compliance_enabled_policies data structureerDiagram
COMPLIANCE_ENABLED_POLICIES {
enabled_policies json
tailoring_policies json
}
ENABLED_POLICY {
id string
title string
description string
business_objective string
compliance_threshold float
total_system_count int
type string
os_major_version int
profile_title string
ref_id string
}
TAILORING_POLICY {
ref_id string
}
CHECK_ITEM {
idref string
selected string
}
COMPLIANCE_ENABLED_POLICIES ||--o{ ENABLED_POLICY : contains
COMPLIANCE_ENABLED_POLICIES ||--o{ TAILORING_POLICY : contains
TAILORING_POLICY ||--o{ CHECK_ITEM : has
Class diagram for the new ComplianceEnablePolicies parserclassDiagram
class ComplianceEnablePolicies {
<<parser>>
+parse(JSON data)
+enabled_policies: list
+tailoring_policies: list
}
ComplianceEnablePolicies --|> JSONParser
JSONParser <|-- ComplianceEnablePolicies
class JSONParser {
<<base class>>
}
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @wushiqinlou - I've reviewed your changes - here's some feedback:
- The
optionallists in os_version and package_check referencecompliance_policies_enabled, but the new spec is registered ascompliance_advisor_rule_enabled_policies—please align the spec names across compliance_ds, Specs, DefaultSpecs, and the parser decorator. - The compliance_advisor_rule_enabled datasource wraps all exceptions and raises SkipComponent, which can obscure real errors—consider catching only the expected exceptions or letting others propagate for easier debugging.
- The new tests include large inline XML/JSON blobs; extracting these into fixtures or external test resources would improve readability and maintainability.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `optional` lists in os_version and package_check reference `compliance_policies_enabled`, but the new spec is registered as `compliance_advisor_rule_enabled_policies`—please align the spec names across compliance_ds, Specs, DefaultSpecs, and the parser decorator.
- The compliance_advisor_rule_enabled datasource wraps all exceptions and raises SkipComponent, which can obscure real errors—consider catching only the expected exceptions or letting others propagate for easier debugging.
- The new tests include large inline XML/JSON blobs; extracting these into fixtures or external test resources would improve readability and maintainability.
## Individual Comments
### Comment 1
<location> `insights/tests/datasources/compliance/test_compliance_ds.py:454` </location>
<code_context>
+ compressor='gz',
+ compliance=False,
+)
+def test_compliance_advisor_rule_enabled_policies(config, policies, tailoring_content):
+ broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config}
+ ret = compliance_advisor_rule_enabled(broker)
+ assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}], "tailoring_policies": [{"ref_id": "foo", "check_items": [{"idref": "xccdf_org.ssgproject.content_rule_bios_disable_usb_boot", "selected": "true"}, {"idref": "xccdf_org.ssgproject.content_rule_ssh_keys_passphrase_protected", "selected": "true"}]}]}']
+ assert ret.relative_path == "insights_datasources/compliance_enabled_policies"
+
+
</code_context>
<issue_to_address>
Missing test for malformed tailoring content in compliance_advisor_rule_enabled.
Add a test where fetch_tailoring_content returns malformed or invalid XML to verify that compliance_advisor_rule_enabled raises SkipComponent on parsing errors.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
+def test_compliance_advisor_rule_enabled_policies(config, policies, tailoring_content):
+ broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config}
+ ret = compliance_advisor_rule_enabled(broker)
+ assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}], "tailoring_policies": [{"ref_id": "foo", "check_items": [{"idref": "xccdf_org.ssgproject.content_rule_bios_disable_usb_boot", "selected": "true"}, {"idref": "xccdf_org.ssgproject.content_rule_ssh_keys_passphrase_protected", "selected": "true"}]}]}']
+ assert ret.relative_path == "insights_datasources/compliance_enabled_policies"
+
+
=======
+def test_compliance_advisor_rule_enabled_policies(config, policies, tailoring_content):
+ broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config}
+ ret = compliance_advisor_rule_enabled(broker)
+ assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}], "tailoring_policies": [{"ref_id": "foo", "check_items": [{"idref": "xccdf_org.ssgproject.content_rule_bios_disable_usb_boot", "selected": "true"}, {"idref": "xccdf_org.ssgproject.content_rule_ssh_keys_passphrase_protected", "selected": "true"}]}]}']
+ assert ret.relative_path == "insights_datasources/compliance_enabled_policies"
+
+
+# Test for malformed tailoring content
+import pytest
+from unittest.mock import patch
+
+def test_compliance_advisor_rule_enabled_malformed_tailoring(config, policies):
+ # Patch fetch_tailoring_content to return malformed XML
+ malformed_xml = "<Tailoring><Broken></Tailoring" # missing closing '>'
+ with patch("insights.datasources.compliance.compliance_ds.fetch_tailoring_content", return_value=malformed_xml):
+ broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config}
+ with pytest.raises(SkipComponent):
+ compliance_advisor_rule_enabled(broker)
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `insights/tests/datasources/compliance/test_compliance_ds.py:495` </location>
<code_context>
+ compressor='gz',
+ compliance=False,
+)
+def test_compliance_advisor_rule_enabled_policies_no_tailoring_policy(config, policies, tailoring_content):
+ broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config}
+ ret = compliance_advisor_rule_enabled(broker)
+ assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}]}']
+ assert ret.relative_path == "insights_datasources/compliance_enabled_policies"
</code_context>
<issue_to_address>
Missing test for multiple policies with mixed tailoring content.
Please add a test where get_system_policies returns multiple policies, with and without tailoring content, to ensure tailoring_policies are included only when content is present.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
+ compressor='gz',
+ compliance=False,
+)
+def test_compliance_advisor_rule_enabled_policies_no_tailoring_policy(config, policies, tailoring_content):
+ broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config}
+ ret = compliance_advisor_rule_enabled(broker)
+ assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}]}']
+ assert ret.relative_path == "insights_datasources/compliance_enabled_policies"
=======
+ compressor='gz',
+ compliance=False,
+)
+def test_compliance_advisor_rule_enabled_policies_no_tailoring_policy(config, policies, tailoring_content):
+ broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config}
+ ret = compliance_advisor_rule_enabled(broker)
+ assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}]}']
+ assert ret.relative_path == "insights_datasources/compliance_enabled_policies"
+
+
+# Test for multiple policies with mixed tailoring content
+import json
+from unittest.mock import patch
+
+@patch(
+ "insights.specs.datasources.compliance.ComplianceClient.get_system_policies",
+)
+@patch(
+ "insights.specs.datasources.compliance.ComplianceClient.get_tailoring_content",
+)
+@patch(
+ "insights.client.config.InsightsConfig",
+ base_url='localhost/app',
+ systemid='',
+ proxy=None,
+ compressor='gz',
+ compliance=False,
+)
+def test_compliance_advisor_rule_enabled_policies_mixed_tailoring(
+ mock_config, mock_get_tailoring_content, mock_get_system_policies
+):
+ # Setup: two policies, one with tailoring, one without
+ policies = [
+ {'ref_id': 'foo', 'id': 'def76af0-9b6f-4b37-ac6c-db61354acbb5'},
+ {'ref_id': 'bar', 'id': 'abc12345-6789-4b37-ac6c-db61354acbb5'}
+ ]
+ tailoring_content = {
+ 'def76af0-9b6f-4b37-ac6c-db61354acbb5': '{"tailoring": "content"}'
+ # No tailoring for 'abc12345-6789-4b37-ac6c-db61354acbb5'
+ }
+ mock_get_system_policies.return_value = policies
+ mock_get_tailoring_content.side_effect = lambda policy_id: tailoring_content.get(policy_id)
+ broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': mock_config}
+ ret = compliance_advisor_rule_enabled(broker)
+ # Parse the JSON output for easier assertions
+ result = json.loads(ret.content[0])
+ enabled_policies = result.get("enabled_policies", [])
+ tailoring_policies = result.get("tailoring_policies", [])
+ # Both policies should be enabled
+ assert {'ref_id': 'foo', 'id': 'def76af0-9b6f-4b37-ac6c-db61354acbb5'} in enabled_policies
+ assert {'ref_id': 'bar', 'id': 'abc12345-6789-4b37-ac6c-db61354acbb5'} in enabled_policies
+ # Only the policy with tailoring content should be in tailoring_policies
+ assert any(p['id'] == 'def76af0-9b6f-4b37-ac6c-db61354acbb5' for p in tailoring_policies)
+ assert all(p['id'] != 'abc12345-6789-4b37-ac6c-db61354acbb5' for p in tailoring_policies)
+ assert ret.relative_path == "insights_datasources/compliance_enabled_policies"
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `insights/tests/parsers/test_compliance_enabled_policies.py:70` </location>
<code_context>
+ assert "Empty output." in str(ex)
+
+
+def test_compliance_enabled_policies():
+ compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(COMPLIANCE_ENABLE_POLICIES))
+ assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1'
+ assert compliance_enabled_policies_info['tailoring_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1'
+ assert len(compliance_enabled_policies_info['enabled_policies']) == 2
+ assert len(compliance_enabled_policies_info['tailoring_policies'][0]['check_items']) == 4
+
+
</code_context>
<issue_to_address>
Missing test for parser with only enabled_policies and no tailoring_policies.
Add a test where the input JSON includes only enabled_policies and omits tailoring_policies to verify correct parser behavior when optional keys are missing.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_compliance_enabled_policies():
compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(COMPLIANCE_ENABLE_POLICIES))
assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1'
assert compliance_enabled_policies_info['tailoring_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1'
assert len(compliance_enabled_policies_info['enabled_policies']) == 2
assert len(compliance_enabled_policies_info['tailoring_policies'][0]['check_items']) == 4
=======
def test_compliance_enabled_policies():
compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(COMPLIANCE_ENABLE_POLICIES))
assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1'
assert compliance_enabled_policies_info['tailoring_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1'
assert len(compliance_enabled_policies_info['enabled_policies']) == 2
assert len(compliance_enabled_policies_info['tailoring_policies'][0]['check_items']) == 4
def test_compliance_enabled_policies_only_enabled():
"""
Test parser with only enabled_policies and no tailoring_policies.
"""
only_enabled_policies = '''
{
"enabled_policies": [
{
"id": "12345678-aaaa-bbbb-cccc-1234567890ab",
"ref_id": "xccdf_org.ssgproject.content_profile_standard"
}
]
}
'''
compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(only_enabled_policies))
assert 'enabled_policies' in compliance_enabled_policies_info
assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_standard'
assert 'tailoring_policies' not in compliance_enabled_policies_info or not compliance_enabled_policies_info.get('tailoring_policies')
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
454
to
465
| def test_compliance_advisor_rule_enabled_policies(config, policies, tailoring_content): | ||
| broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | ||
| ret = compliance_advisor_rule_enabled(broker) | ||
| assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}], "tailoring_policies": [{"ref_id": "foo", "check_items": [{"idref": "xccdf_org.ssgproject.content_rule_bios_disable_usb_boot", "selected": "true"}, {"idref": "xccdf_org.ssgproject.content_rule_ssh_keys_passphrase_protected", "selected": "true"}]}]}'] | ||
| assert ret.relative_path == "insights_datasources/compliance_enabled_policies" | ||
|
|
||
|
|
There was a problem hiding this comment.
suggestion (testing): Missing test for malformed tailoring content in compliance_advisor_rule_enabled.
Add a test where fetch_tailoring_content returns malformed or invalid XML to verify that compliance_advisor_rule_enabled raises SkipComponent on parsing errors.
Suggested change
| def test_compliance_advisor_rule_enabled_policies(config, policies, tailoring_content): | |
| broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | |
| ret = compliance_advisor_rule_enabled(broker) | |
| assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}], "tailoring_policies": [{"ref_id": "foo", "check_items": [{"idref": "xccdf_org.ssgproject.content_rule_bios_disable_usb_boot", "selected": "true"}, {"idref": "xccdf_org.ssgproject.content_rule_ssh_keys_passphrase_protected", "selected": "true"}]}]}'] | |
| assert ret.relative_path == "insights_datasources/compliance_enabled_policies" | |
| +def test_compliance_advisor_rule_enabled_policies(config, policies, tailoring_content): | |
| + broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | |
| + ret = compliance_advisor_rule_enabled(broker) | |
| + assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}], "tailoring_policies": [{"ref_id": "foo", "check_items": [{"idref": "xccdf_org.ssgproject.content_rule_bios_disable_usb_boot", "selected": "true"}, {"idref": "xccdf_org.ssgproject.content_rule_ssh_keys_passphrase_protected", "selected": "true"}]}]}'] | |
| + assert ret.relative_path == "insights_datasources/compliance_enabled_policies" | |
| + | |
| + | |
| +# Test for malformed tailoring content | |
| +import pytest | |
| +from unittest.mock import patch | |
| + | |
| +def test_compliance_advisor_rule_enabled_malformed_tailoring(config, policies): | |
| + # Patch fetch_tailoring_content to return malformed XML | |
| + malformed_xml = "<Tailoring><Broken></Tailoring" # missing closing '>' | |
| + with patch("insights.datasources.compliance.compliance_ds.fetch_tailoring_content", return_value=malformed_xml): | |
| + broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | |
| + with pytest.raises(SkipComponent): | |
| + compliance_advisor_rule_enabled(broker) |
There was a problem hiding this comment.
Please consider this suggestion from sourcery-ai.
Comment on lines
492
to
568
| compressor='gz', | ||
| compliance=False, | ||
| ) | ||
| def test_compliance_advisor_rule_enabled_policies_no_tailoring_policy(config, policies, tailoring_content): | ||
| broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | ||
| ret = compliance_advisor_rule_enabled(broker) | ||
| assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}]}'] | ||
| assert ret.relative_path == "insights_datasources/compliance_enabled_policies" |
There was a problem hiding this comment.
suggestion (testing): Missing test for multiple policies with mixed tailoring content.
Please add a test where get_system_policies returns multiple policies, with and without tailoring content, to ensure tailoring_policies are included only when content is present.
Suggested change
| compressor='gz', | |
| compliance=False, | |
| ) | |
| def test_compliance_advisor_rule_enabled_policies_no_tailoring_policy(config, policies, tailoring_content): | |
| broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | |
| ret = compliance_advisor_rule_enabled(broker) | |
| assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}]}'] | |
| assert ret.relative_path == "insights_datasources/compliance_enabled_policies" | |
| + compressor='gz', | |
| + compliance=False, | |
| +) | |
| +def test_compliance_advisor_rule_enabled_policies_no_tailoring_policy(config, policies, tailoring_content): | |
| + broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | |
| + ret = compliance_advisor_rule_enabled(broker) | |
| + assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}]}'] | |
| + assert ret.relative_path == "insights_datasources/compliance_enabled_policies" | |
| + | |
| + | |
| +# Test for multiple policies with mixed tailoring content | |
| +import json | |
| +from unittest.mock import patch | |
| + | |
| +@patch( | |
| + "insights.specs.datasources.compliance.ComplianceClient.get_system_policies", | |
| +) | |
| +@patch( | |
| + "insights.specs.datasources.compliance.ComplianceClient.get_tailoring_content", | |
| +) | |
| +@patch( | |
| + "insights.client.config.InsightsConfig", | |
| + base_url='localhost/app', | |
| + systemid='', | |
| + proxy=None, | |
| + compressor='gz', | |
| + compliance=False, | |
| +) | |
| +def test_compliance_advisor_rule_enabled_policies_mixed_tailoring( | |
| + mock_config, mock_get_tailoring_content, mock_get_system_policies | |
| +): | |
| + # Setup: two policies, one with tailoring, one without | |
| + policies = [ | |
| + {'ref_id': 'foo', 'id': 'def76af0-9b6f-4b37-ac6c-db61354acbb5'}, | |
| + {'ref_id': 'bar', 'id': 'abc12345-6789-4b37-ac6c-db61354acbb5'} | |
| + ] | |
| + tailoring_content = { | |
| + 'def76af0-9b6f-4b37-ac6c-db61354acbb5': '{"tailoring": "content"}' | |
| + # No tailoring for 'abc12345-6789-4b37-ac6c-db61354acbb5' | |
| + } | |
| + mock_get_system_policies.return_value = policies | |
| + mock_get_tailoring_content.side_effect = lambda policy_id: tailoring_content.get(policy_id) | |
| + broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': mock_config} | |
| + ret = compliance_advisor_rule_enabled(broker) | |
| + # Parse the JSON output for easier assertions | |
| + result = json.loads(ret.content[0]) | |
| + enabled_policies = result.get("enabled_policies", []) | |
| + tailoring_policies = result.get("tailoring_policies", []) | |
| + # Both policies should be enabled | |
| + assert {'ref_id': 'foo', 'id': 'def76af0-9b6f-4b37-ac6c-db61354acbb5'} in enabled_policies | |
| + assert {'ref_id': 'bar', 'id': 'abc12345-6789-4b37-ac6c-db61354acbb5'} in enabled_policies | |
| + # Only the policy with tailoring content should be in tailoring_policies | |
| + assert any(p['id'] == 'def76af0-9b6f-4b37-ac6c-db61354acbb5' for p in tailoring_policies) | |
| + assert all(p['id'] != 'abc12345-6789-4b37-ac6c-db61354acbb5' for p in tailoring_policies) | |
| + assert ret.relative_path == "insights_datasources/compliance_enabled_policies" |
There was a problem hiding this comment.
Please consider this suggestion from sourcery-ai.
Comment on lines
70
to
75
| def test_compliance_enabled_policies(): | ||
| compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(COMPLIANCE_ENABLE_POLICIES)) | ||
| assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1' | ||
| assert compliance_enabled_policies_info['tailoring_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1' | ||
| assert len(compliance_enabled_policies_info['enabled_policies']) == 2 | ||
| assert len(compliance_enabled_policies_info['tailoring_policies'][0]['check_items']) == 4 |
There was a problem hiding this comment.
suggestion (testing): Missing test for parser with only enabled_policies and no tailoring_policies.
Add a test where the input JSON includes only enabled_policies and omits tailoring_policies to verify correct parser behavior when optional keys are missing.
Suggested change
| def test_compliance_enabled_policies(): | |
| compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(COMPLIANCE_ENABLE_POLICIES)) | |
| assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1' | |
| assert compliance_enabled_policies_info['tailoring_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1' | |
| assert len(compliance_enabled_policies_info['enabled_policies']) == 2 | |
| assert len(compliance_enabled_policies_info['tailoring_policies'][0]['check_items']) == 4 | |
| def test_compliance_enabled_policies(): | |
| compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(COMPLIANCE_ENABLE_POLICIES)) | |
| assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1' | |
| assert compliance_enabled_policies_info['tailoring_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1' | |
| assert len(compliance_enabled_policies_info['enabled_policies']) == 2 | |
| assert len(compliance_enabled_policies_info['tailoring_policies'][0]['check_items']) == 4 | |
| def test_compliance_enabled_policies_only_enabled(): | |
| """ | |
| Test parser with only enabled_policies and no tailoring_policies. | |
| """ | |
| only_enabled_policies = ''' | |
| { | |
| "enabled_policies": [ | |
| { | |
| "id": "12345678-aaaa-bbbb-cccc-1234567890ab", | |
| "ref_id": "xccdf_org.ssgproject.content_profile_standard" | |
| } | |
| ] | |
| } | |
| ''' | |
| compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(only_enabled_policies)) | |
| assert 'enabled_policies' in compliance_enabled_policies_info | |
| assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_standard' | |
| assert 'tailoring_policies' not in compliance_enabled_policies_info or not compliance_enabled_policies_info.get('tailoring_policies') |
There was a problem hiding this comment.
Please consider this suggestion from sourcery-ai.
|
|
||
| @datasource(os_version, package_check, HostContext, timeout=0) | ||
| def compliance_advisor_rule_enabled(broker): | ||
| try: |
There was a problem hiding this comment.
issue (code-quality): We've found these issues:
- Use named expression to simplify assignment and conditional (
use-named-expression) - Merge dictionary assignment with declaration [×2] (
merge-dict-assign) - Use f-string instead of string concatenation [×2] (
use-fstring-for-concatenation) - Explicitly raise from a previous error (
raise-from-previous-error)
xiangce
changed the title
xiangce
requested changes
Aug 1, 2025
| { | ||
| "enabled_policies": [ | ||
| { | ||
| "id": "717539de-3c90-473b-acca-c8ee95bb6cc3", |
There was a problem hiding this comment.
Please mock/obfuscate these ids to avoid misunderstanding
| "ref_id": "xccdf_org.ssgproject.content_profile_cis_server_l1" | ||
| }, | ||
| { | ||
| "id": "bc11fd8a-9c76-484c-ac63-14b29414a455", |
There was a problem hiding this comment.
Please mock/obfuscate these ids to avoid misunderstanding
insights/specs/default.py
Outdated
| @@ -115,6 +115,7 @@ class DefaultSpecs(Specs): | |||
| compliance_policies = compliance_ds.compliance_policies | |||
| compliance_assign = compliance_ds.compliance_assign | |||
| compliance_unassign = compliance_ds.compliance_unassign | |||
| compliance_advisor_rule_enabled_policies = compliance_ds.compliance_advisor_rule_enabled | |||
There was a problem hiding this comment.
As this is not a spec belong to the --compliance family, please move it to the next section.
| ) | ||
| @patch( | ||
| "insights.specs.datasources.compliance.ComplianceClient.get_system_policies", | ||
| return_value=[{'ref_id': 'foo', 'id': 'def76af0-9b6f-4b37-ac6c-db61354acbb5'}], |
There was a problem hiding this comment.
Please mock/obfuscate these ids to avoid misunderstanding
| tailoring_policy['check_items'].append(item.attrib) | ||
| result['tailoring_policies'].append(tailoring_policy) | ||
| if not result['tailoring_policies']: | ||
| del result['tailoring_policies'] |
There was a problem hiding this comment.
How about:
tailoring_policies = []
for ...
...
tailoring_policies.append(tailoring_policy)
if tailoring_policies:
result['tailoring_policies'] = tailoring_policiesinstead of adding it then remove it when empty?
| profile_select_tag = pre_tag + 'Profile/' + pre_tag + 'select' | ||
| profile_select_info = xml_root.findall(profile_select_tag) | ||
| for item in profile_select_info: | ||
| tailoring_policy['check_items'].append(item.attrib) |
There was a problem hiding this comment.
How about
tailoring_policy['check_items'] = [item.attrib for item in profile_select_info]
insights/specs/__init__.py
Outdated
| @@ -17,6 +17,7 @@ class Specs(SpecSet): | |||
| compliance_policies = RegistryPoint() | |||
| compliance_assign = RegistryPoint() | |||
| compliance_unassign = RegistryPoint() | |||
| compliance_advisor_rule_enabled_policies = RegistryPoint() | |||
There was a problem hiding this comment.
Better move it the next Regular specs section.
Comment on lines
454
to
465
| def test_compliance_advisor_rule_enabled_policies(config, policies, tailoring_content): | ||
| broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | ||
| ret = compliance_advisor_rule_enabled(broker) | ||
| assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}], "tailoring_policies": [{"ref_id": "foo", "check_items": [{"idref": "xccdf_org.ssgproject.content_rule_bios_disable_usb_boot", "selected": "true"}, {"idref": "xccdf_org.ssgproject.content_rule_ssh_keys_passphrase_protected", "selected": "true"}]}]}'] | ||
| assert ret.relative_path == "insights_datasources/compliance_enabled_policies" | ||
|
|
||
|
|
There was a problem hiding this comment.
Please consider this suggestion from sourcery-ai.
Comment on lines
492
to
568
| compressor='gz', | ||
| compliance=False, | ||
| ) | ||
| def test_compliance_advisor_rule_enabled_policies_no_tailoring_policy(config, policies, tailoring_content): | ||
| broker = {os_version: ['8', '10'], package_check: '0.1.73', 'client_config': config} | ||
| ret = compliance_advisor_rule_enabled(broker) | ||
| assert ret.content == ['{"enabled_policies": [{"ref_id": "foo", "id": "def76af0-9b6f-4b37-ac6c-db61354acbb5"}]}'] | ||
| assert ret.relative_path == "insights_datasources/compliance_enabled_policies" |
There was a problem hiding this comment.
Please consider this suggestion from sourcery-ai.
Comment on lines
70
to
75
| def test_compliance_enabled_policies(): | ||
| compliance_enabled_policies_info = ComplianceEnablePolicies(context_wrap(COMPLIANCE_ENABLE_POLICIES)) | ||
| assert compliance_enabled_policies_info['enabled_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1' | ||
| assert compliance_enabled_policies_info['tailoring_policies'][0]['ref_id'] == 'xccdf_org.ssgproject.content_profile_cis_server_l1' | ||
| assert len(compliance_enabled_policies_info['enabled_policies']) == 2 | ||
| assert len(compliance_enabled_policies_info['tailoring_policies'][0]['check_items']) == 4 |
There was a problem hiding this comment.
Please consider this suggestion from sourcery-ai.
xiangce
changed the title
xiangce
reviewed
Aug 1, 2025
| @@ -0,0 +1,78 @@ | |||
| """ | |||
| ComplianceEnablePolicies - datasource ``compliance_advisor_rule_enabled`` | |||
There was a problem hiding this comment.
Please update the data source/spec/parser name a bit consistent.
wushiqinlou
force-pushed
the
Add_parser_compliance_policies_enabled
branch
4 times, most recently
from
35152ad to
625fe2f
Compare
|
@xiangce - updated per your comments, please confirm again. |
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
Signed-off-by: Jiajun Zhang <[email protected]>
wushiqinlou
force-pushed
the
Add_parser_compliance_policies_enabled
branch
from
9a3be71 to
29d758c
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
All Pull Requests:
Check all that apply:
Complete Description of Additions/Changes:
This parser is used to fix conflict between compliances and advisor rules.
Summary by Sourcery
Introduce a new datasource and parser to fetch and parse enabled compliance policies, and update existing datasources to respect compliance contexts.
New Features:
Enhancements:
Documentation:
Tests: