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: add AutoYaST error reporting #1476

Merged
merged 28 commits into from
Jul 23, 2024
Merged

feat: add AutoYaST error reporting #1476

merged 28 commits into from
Jul 23, 2024

Conversation

jreidinger
Copy link
Contributor

@jreidinger jreidinger commented Jul 18, 2024

Problem

Autoyast conversion script can try to open ui for reporting errors or ask questions.

Solution

Monkey patch it to use agama question mechanism. This change tries to cover the most common cases.
It also needs to implement generic Question with Password react component.
And last but not least during manual testing few issues are revealed and fixed. Also as part of change listing of all question was fixed to really show only unanswered ones and optimize it to use ObjectManager methods.

Testing

For testing CLI for asking this command is used:

agama questions ask < data.json

For testing autoyast questions it uses encrypted autoyast profile that ask for password and also report issue when password is wrong:

agama profile autoyast ftp://pong.suse.cz/autoinst_enc.xml

Copy link
Contributor

@lslezak lslezak left a comment

Choose a reason for hiding this comment

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

👍

@jreidinger jreidinger marked this pull request as ready for review July 22, 2024 15:25
@jreidinger jreidinger changed the title initial version of report patching Autoyast Error Reporting Jul 22, 2024
rust/agama-cli/src/questions.rs Outdated Show resolved Hide resolved
rust/agama-cli/src/questions.rs Outdated Show resolved Hide resolved
@@ -71,11 +72,14 @@ async fn ask_question() -> Result<(), ServiceError> {
let question = serde_json::from_reader(std::io::stdin())?;

let created_question = client.create_question(&question).await?;
let answer = client.get_answer(created_question.generic.id).await?;
let Some(id) = created_question.generic.id else {
return Err(ServiceError::QuestionNotExist(0));
Copy link
Contributor

Choose a reason for hiding this comment

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

What does that '0' mean? It looks like the error is not exactly that the question does not exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be honest this should not happen as it is assigned in that create_question from created question path. I am not sure if we should have something like Programatic error or something that for cases which should not happen and user cannot do naything with it

@@ -19,7 +19,8 @@ pub struct Question {
#[derive(Clone, Serialize, Deserialize, utoipa::ToSchema)]
#[serde(rename_all = "camelCase")]
pub struct GenericQuestion {
pub id: u32,
/// id is optional as newly created questions does not have it assigned
pub id: Option<u32>,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we get rid of D-Bus in the future, we might consider using an uuid instead of dealing with an Option. But that's out of scope.

rust/agama-server/src/questions/web.rs Outdated Show resolved Hide resolved
# rubocop:disable Lint/UnusedMethodArgument
module Yast2
# :nodoc:
class Popup
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see some duplication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, I forget to remove it

def run
# at first construct agama question to display.
text = @label
question = {
Copy link
Contributor

Choose a reason for hiding this comment

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

Asking the question seems like a repetitive process. It may not be needed by now, but we might consider having a helper method/class to do so.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agreed.

service/package/rubygem-agama-yast.changes Outdated Show resolved Hide resolved
web/src/components/questions/QuestionWithPassword.jsx Outdated Show resolved Hide resolved
@imobachgs
Copy link
Contributor

Commit fc249f8 introduces a few improvements to the tests:

  • If you do not need side-effects (using useEffect or interacting with the UI), you do not need the tests to be async.
  • If the test is not async, you should replace find* (which are async) with query*.
  • Testing that the password input is there is kind of redundant (you are using it in the following test).
  • If you do not need the client, prefer plainRender (specific to Agama).

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.

Please, take into account @dgdavid's comment about the test description and you can merge.

web/src/components/questions/QuestionWithPassword.test.jsx Outdated Show resolved Hide resolved
@imobachgs imobachgs changed the title Autoyast Error Reporting feat: add AutoYaST error reporting Jul 23, 2024
Co-authored-by: Imobach González Sosa <[email protected]>
Co-authored-by: David Díaz <[email protected]>
@jreidinger
Copy link
Contributor Author

There is some troubles with PRG cluster affecting also opensuse services. So tests are red due to failed to load testing container. Will merge even with it as previously it was green and change is just some text changes.

@jreidinger jreidinger merged commit ce0c770 into master Jul 23, 2024
1 of 6 checks passed
@jreidinger jreidinger deleted the ay_errors branch July 23, 2024 16:13
@imobachgs imobachgs mentioned this pull request Sep 20, 2024
imobachgs added a commit that referenced this pull request Sep 20, 2024
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.

5 participants