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.

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

Split products #822

Merged
merged 20 commits into from
Oct 31, 2023
Merged

Split products #822

merged 20 commits into from
Oct 31, 2023

Conversation

jreidinger
Copy link
Contributor

@jreidinger jreidinger commented Oct 26, 2023

Problem

All products are now defined inside agama.yaml and modified for each type of live iso.

Solution

Separate products to own files and also allow them to be in separate rpms.
The default web configuration will live in its own file.
Also for easier testing new script to run testsuite in container was added.

TODO: After merge also iso creation needs to be modified to use new rpms.

Testing

  • Added a new unit test
  • Tested manually

service/products.d/ALP-Dolomite.yaml Outdated Show resolved Hide resolved
service/lib/agama/product_reader.rb Outdated Show resolved Hide resolved
service/test/agama/dbus/server_manager_test.rb Outdated Show resolved Hide resolved
service/lib/agama/config.rb Show resolved Hide resolved
service/lib/agama/product_reader.rb Show resolved Hide resolved
service/test/agama/config_test.rb Outdated Show resolved Hide resolved
Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

I like it, it is looking good :-)

service/lib/agama/config.rb Show resolved Hide resolved
service/run_tests_in_container.sh Outdated Show resolved Hide resolved
service/run_tests_in_container.sh Outdated Show resolved Hide resolved
service/run_tests_in_container.sh Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Oct 30, 2023

Coverage Status

coverage: 74.897% (-0.2%) from 75.125% when pulling 7be5524 on split_products into b5920ac on master.

@jreidinger jreidinger marked this pull request as ready for review October 30, 2023 21:47
@@ -0,0 +1,103 @@
- id: ALP-Dolomite
name: SUSE ALP Dolomite
description: 'SUSE ALP Dolomite is a minimum immutable OS core, focused on
Copy link
Contributor

Choose a reason for hiding this comment

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

BTW did you know that this YAML syntax means that the resulting string has no newlines?
https://yaml.org/spec/1.2.2/#732-single-quoted-style

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, but maybe it is good that wrapping will be done as required?

products.d/agama-products-opensuse.spec Outdated Show resolved Hide resolved
products.d/agama-products-opensuse.spec Outdated Show resolved Hide resolved
security:
tpm_luks_open: true
lsm: selinux
available_lsms:
Copy link
Contributor

Choose a reason for hiding this comment

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

I have used available_lsms as an example, grepping the repository and finding no documentation. The only documentation is the code actually using the config data.

Since we're packaging the product data in separate RPMs, it may start to be an issue.
It may be OK if we are the only team actually touching these RPMs.

Related: https://trello.com/c/DkQ0GRqH/192-agama-move-agama-configuration-from-yaml-to-json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeap, documentation is for sure needed. BTW documentation is at https://github.com/openSUSE/agama/blob/master/doc/yaml_config.md we just forgot to update it

Copy link
Contributor

Choose a reason for hiding this comment

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

For now, it is OK to update that document. At some point, we should start thinking about adding a mechanism (based on JSON or YAML or whatever-format-we-use) to validate the configuration.

Copy link
Contributor

@imobachgs imobachgs left a comment

Choose a reason for hiding this comment

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

It looks almost good. I guess that, to make the CI test work, you need to update the workflow to work with the new configuration layout.

-------------------------------------------------------------------
Mon Oct 30 14:38:51 UTC 2023 - Josef Reidinger <jreidinger@suse.com>

- Initial split of products
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add a reference to this PR and/or #602.

@@ -0,0 +1,4 @@
web:
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the content, cockpit.conf or web.conf could be a better name.

@jreidinger jreidinger merged commit e605eb5 into master Oct 31, 2023
14 checks passed
@jreidinger jreidinger deleted the split_products branch October 31, 2023 16:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants