-
Notifications
You must be signed in to change notification settings - Fork 66
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
answerfile: add available choices comment #600
Conversation
Can one of the admins verify this patch? |
Marking as wip as would like to add some tests for the answerfile generation as part of this pr |
607373b
to
23fb3a8
Compare
leapp-ci build |
1 similar comment
leapp-ci build |
Previous "Dialog choice required" turned out to be too cryptic. Besides that possible remediations with leapp answer command for the dialogs are added to report message. Depends-On: oamg/leapp#600
Previous "Dialog choice required" turned out to be too cryptic. Besides that possible remediations with leapp answer command for the dialogs are added to report message. Depends-On: oamg/leapp#600
77c05dd
to
5b817c1
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.
Just passed an upgrade with the following answerfile:
[remove_pam_pkcs11_module_check]
option = Lol
I don't know the specifics of how the values are checked or handled but it's obviously a blocker.
Why a blocker? How the option is treated is up to actor to decide. It is the actor that checks what value is there and if\how to proceed. |
Alright, I just thought the framework at least checks the types or compares available options. Regardless, I'll put this under the actors PR. I don't think this should pass without errors or any feedback to the user. |
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.
Looks good to me.
ea2e0fa
to
76ed50a
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.
With the current approach, i see here really bad user experience. There should not be any lower/higher priority at all. In this case, we should not read anything from the answerfile. We should read data only from the answer.userchoices
as changes in the answerfile are still lost when user edit it manually. To be honest, this makes much bigger mess than the other solution we discussed. So, to make it mergable:
- all docs has to be updated and user must not update answerfile manualy, but only by
leapp answer
- or all docs has to be updated as user will be required to copy the answerfile (or its parts) to the
answer.userchoices
file or use theleapp answer
- leapp has to read only from the
answer.userchoices
The another solution I mentioned was:
- load the asnwerfile to the answerstore (as now)
- create copy of answerstore keeping the original answers
- processing of workflow is same
- when leapp generate answerfile, provide even the original answerstore to the function
- in the function generating answerfile:
-- 1 if dialog is produced by actors, use that one
-- 2 if dialog in orig answerstore is not produced by actors, store the original
In such case, only one file is needed. In case that there is dialog in the answerfile already that is not relevant for the system or e.g. we removed the need of the dialog in actors, dialog will be still in the answerfile, unused, doing nothing, forever. But I do not see here the different between to keep unuseed dialog in the answerfile and keep unused question in the answer.userchoices
file.. As well, the step 2) in the second solution can be little improved, when we will keep only answered dialogs - so e.g. in case of development, if people will be changing the dialog id, they will not have to remove it manually, e.g.
EDIT: 3rd solution represented in comments below. That is combing good UX an clean answerfile.
Btw, I really thing that the second solution is much better from the UX POV. I would like to know opinion of others as well, just to know whether both solutions are ok (or another new one), or whether specific solution is required. especiall @shaded-enmity , what do you think? Are you ok with both solutions? Guys? @oamg/developers Just to be clear, I am ok with both solutions, but I think the one with only one file is better UX. |
@pirat89 your solution is inappropriate as the produced answerfile after leapp preupgrade run will have absolutely zero resemblance to what actually happened in the workflow (meaning what dialogs were actually raised). This handicaps us greatly - no way to do backwards mapping from the answerfile to the dialogs that were\should be answered, no possibility to include answerfile in the archived logs to ease debugging as it is irrelevant etc. I kept the possibility to change the answerfile as the "demo version" scenario - you can manually change the file via some editor and run the preupgrade with non-persistent changes to the userchoices. But I can drop this if that behavior looks too complicated and mention that only leapp answer command should be used. |
@fernflower I do not agree that there is anything so much bad about that. To have dialogs that are not in workflow, is ok as nothing happens. You will have answer on something that doesn't exist in repo; but what is problem? Such answers will not be used by any actor. Give me example where this is problem. I really do not see it. Now, I can create very same thing using the userchoice file. I wil lhave answers, that are not mapped to anything. I do not see any issue around that. |
@pirat89 I already did - you are killing the 1-1 mapping between the "dialogs actually shown in the workflow" and "answers stored in the answerfile". Imho persistent userchoices and the answerfile are entities of different semantics, with the first one being a "knowledge db that can store all the answers to all the questions even if they are not asked in a specific workflow" and the second one being "just a collection of answers to specific dialogs raised during specific leapp preupgrade run". The first one is permanent, second is temporary and leapp-run-bound. |
@fernflower But explain me please why this is important. I do not see why it's so important to keep 1:1 mapping. I want to understand it why it is important, but I don't. I am still missing the point. |
@fernflower I found third way that would be ok for you as well:
Just to be clear, temporary answer doesn't exist. That's misconception. When user answer question, there is no way to ask for the answer again. |
I've checked the current solution and I actually really like Petr's proposal. But, when regenerating the answerfile, the answers from the userchoice files should be filled in the answerfile. Because otherwise I believe it would be like this: 1. User runs preupgrade and fills in the answers to the answerfile, 2. They run it again and the answerfile is "empty" with no dialogs answered, 3. They would start panicking, not knowing the answers moved to the userchoice files. But please, generate the *.userchoice to a subfolder, like |
Why are you saying that the answer file will be empty? In the solution I have now the answer file in case of proper invocation of leapp preupgrade command is always non-empty as it is generated from the current answer file + persistent user choices from userchoices file. |
@fernflower the answer.userchoices file - I already explain that Michal. it's clear to him now. |
That's not correct either. See:
The answerfile is empty now. |
You didn’t get to the dialogs stage and answerfile maps 1-1 to the dialogs you encountered. But if you rerun the preupgrade the next time with the proper invocation you will see that all the questions you have answered previously are respected. That’s the point, isn’t it - the user is not asked twice for anything they have already answered AND you get a lot of debug information (like that you failed even before any actor with dialogs) just by looking at the answerfile. |
- previously the generated answerfile didn't contain the expected choices for the boolean component (the only one allowed component for actor dialogs so far); - leapp answer command now prints usage message in case of inappropriate invocation; - answerfile path shown at the end of leapp preupgrade run even without --debug.
Earlier DialogModel had answerfile_sections as string field which made it impossible to register possible dialog choice. This commit switches to fields.JSON() usage which should improve overall report readability and dialog info manipulation in verifydialogs actor.
Introduced a persistent between userchoices file that will serve as a knowledge db for registered user options for a specific dialog. To register a persistent user choice you can use leapp answer command. Existing answerfile will be always updated with data from useroptions file before the workflow.run is called to ensure that users are not asked twice for the answer to the question they have already answered. Docs have been updated. Closes-Bug: oamg#605 Closes-Bug: oamg#606
d8a5d5f
to
ad9ad32
Compare
This is a final touch to implementing the persistent answerfile while still keeping the 1-1 mapping of answerfile-workflow dialogs and allowing user to choose both leapp answer tool or editor of choice for answerfile management. The basic idea is refactor load_answerfile\save_answerfile into load_answers\save_answers methods that apart from loading\saving actual answerfile are also dumping the current state of answerstore to the .userchoices file. The leapp answer command will be used for answerfile management as before. oamg#605
ad9ad32
to
28f40e4
Compare
Previous "Dialog choice required" turned out to be too cryptic. Besides that possible remediations with leapp answer command for the dialogs are added to report message. Depends-On: oamg/leapp#600
leapp-repository seems need to be update:
See the PR oamg/leapp-repository#442 |
How are you provisioning the vm? You need both framework patch 600 and leapp-repository 442 alongwith leapp-repository 441. |
I see :) I missed that info in the PR. Thanks. |
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.
Let's celebrate!!! It works! Thanks :) Just cosmetic nitpicks here, but can be resolved in another PRs. Merging.
Previous "Dialog choice required" turned out to be too cryptic. Besides that possible remediations with leapp answer command for the dialogs are added to report message. Depends-On: oamg/leapp#600
## Packaging - Add dependency on python2-leapp and leapp-framework - Add python-requests as dependency (oamg#407) - Drop leapp sos plugin (it's part of the sos rpm in RHEL 7.7+) - Remove dependency on Jinja2 related packages (oamg#407) ## Fixes - Do not mount pseudo and unsupposrted FS to overlayfs (e.g. proc) - Evaluate PES events transitively to create correct data for the upgrade transaction - Fix boot order on EFI systems - Fix checking of kernel drivers (oamg#400) - Fix failures caused by local rpms added into the upgrade transaction - Fix getting mount information with mountpoints with spaces in the path - Fix handling of XFS without ftype for every such mounted FS - Fix issue with random booting into old RHEL 7 kernel after the upgrade - Fix issues on systems with statically mapped IPs in /etc/hosts - Fix issues with device mapper and udev in a container - Fix issues with failing rpm transaction calculation because of duplicate instructions for dnf - Fix various issues related to RHSM (e.g. https://bugzilla.redhat.com/show_bug.cgi?id=1702691) - Fix yum repository scan in case of repositories with invalid URL - Inhibit the upgrade if multiple kernel-devel rpms are installed - Inhibit the upgrade when links on root dir '/' are not absolute to save the world - Parse correctly kernel cmdline inside the initrd (oamg#383) (fixes various issues on s390x) - Print warnings instead of a hard failure when expected rpms cannot be found (e.g. python3-nss inside an rpm module) (oamg#405) - Remove java11-openjdk-headless during the upgrade (https://bugzilla.redhat.com/show_bug.cgi?id=1820172) - Throw a nice error when invalid locale is set (oamg#430) ## Enhancements - Add initial multipath support (it doesn't handle all cases yet) - Changed upgrade paths: RHEL-ALT 7.6 -> 8.2; RHEL 7.8 -> 8.2 - Check if the latest installed kernel is booted before the upgrade - Check that the system satisfies minimum memory requirements for the upgrade (oamg#413) - Dump `grub2-editenv list` output to help with issues related to the default kernel for the boot - Improved report related to KDE/GNOME - Inhibit the upgrade for ipa-server (oamg#481) - Inhibit the upgrade on EFI systems when efibootmgr is not installed - Inhibit the upgrade on FIPS systems - Inhibit the upgrade when the raised dialogs are missing answers (oamg#589) - Introduce new ways of using custom repositories during the transaction - Make report messages more explicit about Dialogs (oamg#600) - Migrate SpamAssassin - Migrate cups-filters - Migrate sane-backend - Modify vim configuration to keep the original behaviour - Report changes in wireshark - Support the upgrade without the use of subscription-manager - The name and baseurl field in the CustomTargetRepository message are optional now - Use the new framework mechanism to inhibit the upgrade without reporting errors - Various texts are improved based on the feedback ## Additional changes interesting for devels - Add new functions in the config library to get envars related to leapp - Add support for testing with Beta and HTB systems - LEAPP_SKIP_CHECK_OS_RELEASE has been renamed to LEAPP_DEVEL_SKIP_CHECK_OS_RELEASE - Provide info about kernel cmdline via KernelCmdline message - The IPUConfig message contains information about booted kernel - The code is mostly Py2/Py3 compatible now and all PRs are tested on Py2 and Py3 compatibility (linters, unit-tests) - The config.version library contains is_rhel_alt() for detection of RHEL-ALT
## Packaging - Bump required leapp-framework capability to 1.4 (oamg#642) ## Upgrade handling ### Fixes - Fix comparison of the newest installed and booted kernel (oamg#600) - Fix remediation command for ipa-server removal (oamg#617) - Fix crash due to missing network interfaces during upgrade phases (oamg#625) - Fix error with /boot/efi existing on non-EFI systems (oamg#627) - Fix false positive detection of issue in /etc/default/grub that led into GRUB prompt (oamg#587) - Fix syntax error in upgrade script (oamg#619) - Inhibit upgrade with mount options in fstab that break mounting on RHEL 8 (oamg#639) - Inhibit upgrade on s390x machines with /boot on a separate partition (oamg#641) - Inhibit upgrade if multiple kernel-debug pkgs are installed (oamg#599) - Remove the initial-setup package to avoid it asking for EULA acceptance during upgrade (oamg#626) - Remove the *leapp-resume* service after the *FirstBoot* phase to prevent kill of the leapp process on `systemctl daemon-reload` (oamg#611) ### Enhancements - Add upgrade support for SAP HANA (own upgrade path) (oamg#503) - Allow upgrade with SCA enabled manifest (oamg#615) - Add actors to migrate Quagga to FRR (oamg#467) - Add stable uniq Key id for every dialog (oamg#618) - Respect the *kernel-rt* package (oamg#600) ## Additional changes interesting for devels - Add a possibility to overwrite virtualenv name using `$VENVNAME` (oamg#613) - Update product certificates for RHEL 8.3 GA and 8.4 Beta/HTB (oamg#624) Related leapp release: https://github.com/oamg/leapp/releases/tag/v0.12.0
Previously the generated answerfile didn't contain the
expected choices for the boolean component (the only one
allowed component for actor dialogs so far).