-
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
Use distributed product properties #10554
Conversation
Skipping CI for Draft Pull Request. |
f0849c0
to
7e53bcc
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.
great idea!
2fe1174
to
3a40f80
Compare
Use this functionality to associate product properties with product versions, | ||
so you can use only product properties in the content. | ||
In other words, use this functionality to avoid referencing product versions in macros used in checks or remediations. | ||
Instead, use properties that directly relate to configurations being checked or set, and that help to reveal the intention of the check or remediation code. |
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 now way better!
We probably should have a blog post in which we would show the "good" and "bad" examples.
ssg/products.py
Outdated
data.update(self._acquired_data) | ||
data.update(self._primary_data) |
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 great to print an error if there are multiple definitions of the same property.
product_properties/README.md
Outdated
Processing of those files can use `jinja2`, and macros or conditionals have access to product properties defined previously and in the `product.yml`. | ||
Properties read from these yaml files can be overriden by properties from files processed later, while the properties specified in the `product.yml` override all previous definitions. | ||
|
||
Conventionally, use the filename prefix `10-` to define defaults, and use subsequent prefixes to override those defaults based on 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.
I don't like it, I would prefer to have a single source of truth, i.e. define a given option only in 1 file.
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 tricky: YAML requires that in mappings, keys have to be unique, so setting a value to the default and redefining it later isn't legal, although libraries may support it.
The idea to define everything in one file is reasonable to me, so here are the options:
What is illegal:
key: value
key: another-value
Dedicated default section:
default:
key: value
overrides:
key: another-value
while the build system has to write overrides over defaults.
List of mappings:
- key: value
- key: another-value
while the build system has to assemble the set of key-value mappings gradually by iterating over the list.
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 that the first option can be unreliable and the third option can be unexpected by newcomers so I prefer the second option because it's explicit. But let's take a look how it fits into the whole context of the "product properties" feature.
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.
What are the benefits of having the key overrides in centrally placed lexicographically ordered files?
Why not keep a centrally defined default, and each product overrides properties in its own product.yml
, or maybe a product property file in the product directory, like products/rhel8/product_properties.yml
?
This also facilitates the addition and removal of a product.
For example, one can copy the file with the default values, and tweak / remove 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.
What are the benefits of having the key overrides in centrally placed lexicographically ordered files? Why not keep a centrally defined default, and each product overrides properties in its own
product.yml
, or maybe a product property file in the product directory, likeproducts/rhel8/product_properties.yml
? This also facilitates the addition and removal of a product. For example, one can copy the file with the default values, and tweak / remove keys.
Sorry, race condition. The override possibility is no longer considered, and if we introduce core product attributes s.a. "family" (ubuntu, debian, ol, rhel) and major version (8, 2204), we may even make the core product properties available at other properties definition time, which would take the ordering away completely.
product_properties/README.md
Outdated
Processing of those files can use `jinja2`, and macros or conditionals have access to product properties defined previously and in the `product.yml`. | ||
Properties read from these yaml files can be overriden by properties from files processed later, while the properties specified in the `product.yml` override all previous definitions. | ||
|
||
Conventionally, use the filename prefix `10-` to define defaults, and use subsequent prefixes to override those defaults based on 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.
What are the benefits of having the key overrides in centrally placed lexicographically ordered files?
Why not keep a centrally defined default, and each product overrides properties in its own product.yml
, or maybe a product property file in the product directory, like products/rhel8/product_properties.yml
?
This also facilitates the addition and removal of a product.
For example, one can copy the file with the default values, and tweak / remove keys.
The code has been altered to use the default/overrides keys, and tests and docs have been updated accordingly, and I believe that questions were addressed. |
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 like this change very much. I believe it will help us keep the data better organized and it will make the content more approachable to the authors.
I have tried to change some keys or override some keys and the behavior I observed was expected.
Please resolve the CodeClimate problem and take a look at the directory structure docs.
Product properties can be expanded by other means, so the only way how to make sure that they remain stable is to check references for stability.
- Assign the grub helper exec to the product - Remove most of the references to product names from grub macros - Substitute literal invocations of e.g. grubby with macros
Grubby is not used in the context of grub, but in another context of BLS utility, and the macro doesn't understand this.
@ComplianceAsCode/ubuntu-maintainers @ComplianceAsCode/suse-maintainers @ComplianceAsCode/oracle-maintainers Can you take a look, please? |
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.
Changes affecting OL products LGTM.
I like this new way to set product properties. Thanks for the efforts @matejak!
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 Ubuntu changes lgtm, thanks for doing this!
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.
Besides the grubby vs grub2-mkconfig, I think rest of the changes are OK on SLE platforms. Nice improvement 🙇
@matejak ping |
Code Climate has analyzed commit 1ca395c and detected 0 issues on this pull request. 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 53.2% (0.4% change). View more on Code Climate. |
@teacup-on-rockingchair Can you check if the latest changes feel accurate for you to approve this PR? |
@matejak PTAL at @marcusburghardt comment |
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.
LGTM
I reviewed the PR again now and confirmed that all pending comments were properly managed. I marked some simpler comments as resolved while kept others open because they have useful information. This is new to the project and improvements are naturally expected along the time but the current state is pretty good to land. We can merge it and have its benefits already in 0.1.69 release. |
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.
Great stuff 🙇
Description:
The PR builds on top of #10529. After that one is merged, this should get rebased in order to become easily reviewable.It introduces product properties that can be read from a directory that can contain jinja2-powered yaml files that are processed in lexicographic order.
During processing of those files, they have access to the existing product properties. Properties read from those yaml files can be overriden by later files, while the properties specified in product yaml always stay on top.
Rationale:
This change will allow to move aspects of products from constants and source code in general to human-accessible yaml files in a top-level directory.
Review Hints: