search

oamg / leapp

Leapp - Application & OS Modernization Framework (For in-place upgrades, follow leapp-repository)
https://oamg.github.io/leapp/
Apache License 2.0
87 stars 70 forks source link

Dialog Component - Add TextListComponent to support list of text #833

Closed ygalblum closed 1 year ago

ygalblum commented 1 year ago

Add a new Component to hold a list of strings with a minimal count Implement a specific renderer hook Load and dump the list from and to the answer file as a json Add test cases for the dialog components Add test cases for the answerstore

github-actions[bot] commented 1 year ago

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable. If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

To launch regression testing public members of oamg organization can leave the following comment:

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please consider rerunning the CI by commenting leapp-ci build (might require several comments). If the problem persists, contact leapp-infra.

ygalblum commented 1 year ago

review please @oamg/developers

PeterMocary commented 1 year ago

Hi, thank you for your contribution @ygalblum! :rocket:

Could you please update the description with some additional information about the reason behind the changes?

pirat89 commented 1 year ago

@ygalblum @PeterMocary we need to discuss this yet as the use case behind this feature is aside of the original purpose of Dialogs. Please do not do other changes until we discuss that. We are now affected by vacations so waiting for a time most of the team is back to discuss it.

A use case behind the PR:

The possible solution (that is nowadays more expected regarding the dialogs design) is to tell user to create a file under /etc/leapp/files/ (report inhibitor when the file does not exist) that will address such a problem. In this case actors can simply read the content of such a configuration file and work with it as needed. (@ygalblum what do you think about that solution?)

Also to be honest, I do not like the idea to include additional format inside the answerfile file and I still feel that the use case is not following our expectations from answerfile, so I am tentatively against this change. But I will still think about that and will wait for the team discussion.

@ygalblum Regardless the decision we will do, thank you for the contribution.

ygalblum commented 1 year ago

@pirat89 thanks for the detailed comment and yes, you are correct, this was the reason for this PR.

The reason I prefer this solution over your proposal is that I think that users will prefer to provide all the information in one place instead of multiple files. Don't think only about my actor. Once this becomes a known practice, what stops the next actor from getting answers in another file?

As for the way it was implemented, i.e. JSON inside an INI file, I looked around for how arrays are implemented in INI files and this was the most common and elegant way I found.

Having said that, because of your response (in the chat) and knowing that even if this change is merged, it will take time until it gets released, I decided to go with a hybrid solution. That is, the actor uses a JSON file to get the information it needs, but it does not force a location. Instead, it uses a Text dialog to get a path or URL to get the json from.

The advantage I saw in this solution is that it allows the user to maintain this JSON file next to where the files, to which the URLs are needed to being with, are located.

pirat89 commented 1 year ago

@ygalblum We have discussed this in the team and we agreed this is not way we want to go in future. We want to keep dialogs for purposes they were designed and do not want to extend them in this way. Also we do not want to mix multiple formats in one file - but this was just a minor fact for the decision as that part could be done differently. In this case we suggest to go the way I've proposed above - e.g.

Closing, we are sorry about that but thanks for your time and the contribution.