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

feat: report unsupported AutoYaST elements #1976

Open
wants to merge 25 commits into
base: master
Choose a base branch
from
Open

Conversation

imobachgs
Copy link
Contributor

@imobachgs imobachgs commented Feb 5, 2025

Problem

Agama does not offer information about which elements are supported when importing an AutoYaST profile and which ones are just ignored.

https://trello.com/c/0u2wwMN2

Solution

Agama checks the profile for unsupported elements. If it finds them, it displays a question listing them and asking the user whether to continue.

Generating the documentation

The mechanism to detect unsupported elements includes a list of all AutoYaST elements and whether they are supported: "yes", "planned", "partial" or "no". The file also contains hints about replacing some elements (e.g., using a post-script).

Although it was out of scope, this PR introduces a new doc:autoyast_compat which generates a reference to be included in the AutoYaST support documentation. Not all elements are included, but we can create a follow-up PBI to work on that.

Disabling the check

It is possible to disable the check specifying agama.ay_check=0 on kernel's command line. Actually, the check is done, but it does not report found problems.

Testing

  • Added a new unit test
  • Tested manually

Screenshots

unsupported-autoyast-popup

Below is an example of the AutoYaST compatibility documentation automatically generated by the new doc:autoyast_compat task.

agama-autoyast-compatibility

To do

  • Improve how the error is displayed.
  • Allow disabling the check.
  • Add all (or at least most of them) the AutoYaST elements to the autoyast-compat.json file.

@imobachgs imobachgs force-pushed the profile-checker branch 2 times, most recently from 80a1704 to 4e515bd Compare February 7, 2025 14:18
@imobachgs imobachgs marked this pull request as ready for review February 7, 2025 14:40
@coveralls
Copy link

Pull Request Test Coverage Report for Build 13202155965

Details

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • 9 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.02%) to 70.888%

Files with Coverage Reduction New Missed Lines %
service/lib/agama/autoyast/connections_reader.rb 4 93.75%
rust/agama-server/tests/common/mod.rs 5 74.19%
Totals Coverage Status
Change from base Build 13198839713: -0.02%
Covered Lines: 17291
Relevant Lines: 24392

💛 - Coveralls

Copy link
Contributor

@mvidner mvidner left a comment

Choose a reason for hiding this comment

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

Comments based so far on the description only, not on code.

  1. Agama checks the profile for unsupported elements.

    When does this happen? Only when I try to use the profile, or can I trigger it beforehand, on a dry run?

  2. "Not supported AY elements" popup

    I think it would be nice to also include this text.

    This popup can be disabled by passing agama.ay_check=0 on the kernel command line

@mvidner
Copy link
Contributor

mvidner commented Feb 7, 2025

2. "Not supported AY elements" popup

Also IIUC the popup displays the "planned" and "no" categories. What about "partial"?

@@ -53,5 +54,9 @@ export default function Questions(): React.ReactNode {
QuestionComponent = LuksActivationQuestion;
}

if (currentQuestion.class === "autoyast.unsupported") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a table item to doc/questions.md

web/src/components/questions/UnsupportedAutoYaST.tsx Outdated Show resolved Hide resolved
module AutoYaST
# This class checks an AutoYaST profile and determines which unsupported elements are used.
#
# It does not report unknown elements.
Copy link
Contributor

Choose a reason for hiding this comment

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

So what happens with unknown elements? Are they silently ignored now or does a different part of Agama report them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to ignore them by now and report only the status of the known elements. The idea is to extend the compat file to cover all the AutoYaST elements, stating whether it is unsupported, supported or planned.

Co-authored-by: Martin Vidner <mvidner@suse.cz>
@imobachgs
Copy link
Contributor Author

2. "Not supported AY elements" popup

Also IIUC the popup displays the "planned" and "no" categories. What about "partial"?

The "partially supported" elements are meant for container elements. When you check the profile, they are not relevant: the important part is the element within. For instance, if you have the following code:

<networking>
  <backend>wicked</backend>
</networking>

Agama will report that networking/backend is unsupported, regardless of the networking status.

However, the "partial" category can be used when generating the AutoYaST compatibility documentation.

@imobachgs
Copy link
Contributor Author

Comments based so far on the description only, not on code.

1. Agama checks the profile for unsupported elements.
   When does this happen? Only when I try to use the profile, or can I trigger it beforehand, on a dry run?

Well, it happens automatically when using an AutoYaST profile (agama.auto=) in kernel's command line. However, I use the following command:

agama-autoyast http://192.168.122.10/autoyast/wrong.xml autoinst

IMHO, agama-autoyast should be "hidden" (probably placed on /usr/libexec) and we should extend the CLI to allow this operation. What do you think?

2. "Not supported AY elements" popup
   I think it would be nice to also include this text.
   > This popup can be disabled by passing agama.ay_check=0 on the kernel command line

Good point.

Copy link
Contributor

@dgdavid dgdavid left a comment

Choose a reason for hiding this comment

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

Interface Code LGTM

@dgdavid
Copy link
Contributor

dgdavid commented Feb 7, 2025

2. "Not supported AY elements" popup
I think it would be nice to also include this text.
> This popup can be disabled by passing agama.ay_check=0 on the kernel command line

+1

and remove 'support: yes' from users[], it is implicit on all items with
'children'
zypper install python3-jsonschema.rpm
jsonschema -i autoyast-compat.json autoyast-compat.schema.json

TODO: check in CI, maybe make `agama profile validate` a bit more
generic
@@ -68,6 +68,7 @@
{ "key": "description", "support": "no" },
{
"key": "bootproto",
"support": "no",
Copy link
Contributor

Choose a reason for hiding this comment

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

@imobachgs Creating the schema did help finding the missing support here. Yay! Is no right?

@dgdavid

This comment was marked as resolved.

@mvidner
Copy link
Contributor

mvidner commented Feb 10, 2025

In today's review you mentioned the problem of keeping the JSON in sync with the code.
I know that automating the synchronization would be hard... can we at least detect automatically that the JSON and the implementation have gone out of sync?

@imobachgs

This comment was marked as resolved.

@dgdavid

This comment was marked as resolved.

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