Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor templates v2 #9870

Merged
merged 13 commits into from
Nov 25, 2022
Merged

Refactor templates v2 #9870

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
f6dfe66
Refactor template builder
evgenyz Nov 16, 2022
219ea25
Refactor templates and XCCDFEntities
evgenyz Nov 24, 2022
395af0e
Refactor templates: Rule and Templatable and products
evgenyz Nov 24, 2022
b7ff233
Refactor templates: move XCCDFEntity-specific code into Templatable
evgenyz Nov 24, 2022
3e0d2c0
Refactor build_yaml
evgenyz Nov 24, 2022
b19366e
Refactor templates
evgenyz Nov 25, 2022
3952bda
Refactor common.Templatable
evgenyz Nov 25, 2022
35e075b
Refactor templates
evgenyz Nov 25, 2022
5c7cf43
Refactor entities/common
evgenyz Nov 25, 2022
c0b411e
Add max_line_length parameter to .editorconfig
evgenyz Nov 25, 2022
cc7ecd7
Refactor entities/common
evgenyz Nov 25, 2022
24be147
Fix line length in build_sce.py
evgenyz Nov 25, 2022
85e3553
Remove offending stigid from apple_os/auditing/service_com_apple_audi…
evgenyz Nov 25, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ssg/entities/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@
def make_items_product_specific(items_dict, product_suffix, allow_overwrites=False):
new_items = dict()
for full_label, value in items_dict.items():
if "@" not in full_label and full_label not in new_items:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a modification of condition without any compensation, so no wonder that it may change the behavior of the function, as reported in #9894
new_items is not an empty dict for the whole time, but the items_dict ordering is different between Python2 and Python3, so the loop may behave differently in those interpreters.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it is the case in that issue - pkgname@rhel7: pam_pkcs11 is supposed to take precedence over pkgname: openssl-pkcs11. What happened probably was that new_items["pkgname"] was set to pam_pkcs11, and it got overwritten by openssl-pkcs11. The condition protected product-qualified items from being overwritten by generic items.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It feels like this case should fall right into https://github.com/evgenyz/content/blob/5c7cf430a1012bf3151872519f7873b99218f5cb/ssg/entities/common.py#L46 instead of being short-circuited.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Otherwise this no-overwrite policy depends on the order of dictionary items (which is not ordered in any way by definition).

Copy link
Member Author

@evgenyz evgenyz Nov 29, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it formulated anywhere besides the exception message? I can't find anything in the docs. Maybe I don't even understand it to the full extent.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The funny thing is that CodeClimate was right about that method being too cognitively complex. I think (may be wrong though) that

        if not full_label.endswith(product_suffix):
            continue

is triggered in case that there is an override attempt of a product-qualified value by a generic value, so now it looks like to me that the exception is not thrown, but the value also isn't overwritten, so it works fine on Python3.
Anyway, only exhaustive unit-testing could bring sufficient level of confidence, and although we would like that, it's out of scope of this PR. The truth is that removing the second part of that conditional was a mistake, and the best thing to do is to fix it - we all know that anybody can make mistakes, and this one was a very educational one that plenty of people can learn from.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The truth is that it ain't over yet: #9910

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

if not full_label.endswith(product_suffix):
            continue

happens after the possible exception.

Python 2 and 3 difference is in the order of items storage/emission. I've reproduced it in a test case using update().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think that the exception happened after this sequence. FTR, the original code can be viewed here:

content/ssg/build_yaml.py

Line 949 in 219ea25

def _make_items_product_specific(self, items_dict, product_suffix, allow_overwrites=False):

The issue #9894 was about product-qualified package, which is definitely not a "global reference". Therefore, the next line if label in items_dict and not allow_overwrites and value != items_dict[label] was one that could trigger an exception, but I don't recall our policy regarding allow_overwrites. That clause probably aims to enforce existence of either only product-qualified specs, or no product-qualified specs at all.
Naming sucks, but at least the commit messages that explain why the function got into that shape are quite solid.

Copy link
Member Author

@evgenyz evgenyz Dec 1, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function has not been used for de-qualifying template variables before refactoring.

if "@" not in full_label:
new_items[full_label] = value
continue

Expand Down Expand Up @@ -53,6 +53,7 @@ def make_items_product_specific(items_dict, product_suffix, allow_overwrites=Fal
value_q=value, value_u=items_dict[label])
)
raise ValueError(msg)

new_items[label] = value
return new_items

Expand Down