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

Fix TypeError during JSON serialization in dialogs #760

Merged
merged 2 commits into from
Feb 28, 2022

Conversation

fernflower
Copy link
Member

Previously a DictProxy entry was passed as part of
{dialog_section: entry} data to save in store which
failed during serialization.

@github-actions
Copy link

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 mergable.
If you want to re-run tests or request review, you can use following commands as a comment:

  • leapp-ci build to run copr build and e2e tests in OAMG CI
  • review please to notify leapp developers of review request
  • /packit copr-build to submit a public copr build using packit

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.

@centos-ci
Copy link

Can one of the admins verify this patch?

@fernflower
Copy link
Member Author

fernflower commented Feb 25, 2022

Hmm I see now that though this solves a general case, this doesn't fix the issue with the following configuration
answerfile

[check_vdo]
# totally empty
#
answerfile.userchoices
[check_vdo]
no_vdo_devices = False

The failure happens elsewhere, looking

@pirat89
Copy link
Member

pirat89 commented Feb 25, 2022

/rerun

@fernflower
Copy link
Member Author

/rerun

@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/3554498

@github-actions
Copy link

Testing Farm request for 7to8/3544828,3554498 regression testing has been created. Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for 8to9/3544828,3554498 regression testing has been created. Once finished, results should be available here.
Full pipeline log.

Previously a DictProxy entry was passed as part of
{dialog_section: entry} data to save in answerstore which
failed during serialization.
This commit fixes AnswerStore.get() method by making it always
return a shallow copy of a DictProxy object.
Also remove all dict() conversions for store.get() calls in
dialog module.
@fernflower
Copy link
Member Author

/rerun

@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/3554614

@github-actions
Copy link

Testing Farm request for 7to8/3544828,3554614 regression testing has been created. Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for 8to9/3544828,3554614 regression testing has been created. Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/3554581

@github-actions
Copy link

Testing Farm request for 7to8/3544828,3554581 regression testing has been created. Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for 8to9/3544828,3554581 regression testing has been created. Once finished, results should be available here.
Full pipeline log.

Copy link
Member

@pirat89 pirat89 left a comment

Choose a reason for hiding this comment

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

works good except the problem mentioned above. but that's a different issue already present probably for a longer time. We do not expect people to remove the key = value line from the ansewrfile, keeping the section defined. This solution fixes the general problem.

@fernflower bumped leapp-framework = 2.2 so we could start to require it in leapp-upgrade packages to ommit possible issues when people could test leapp-upgrade builds with dialogs but older leapp builds. Feel free to squash it directly into the one commit (i didn't want to touch the orig one as tests are running for it already).

@fernflower
Copy link
Member Author

fernflower commented Feb 28, 2022

works good except the problem mentioned above. but that's a different issue already present probably for a longer time. We do not expect people to remove the key = value line from the ansewrfile, keeping the section defined. This solution fixes the general problem.

@pirat89 what do you mean by "except the problem mentioned above"? If you are referring to my comment #760 (comment) then it is obsolete, the current version of the patch resolves this issue as well. Serialization problems should be solved in all cases now.

@pirat89
Copy link
Member

pirat89 commented Feb 28, 2022

@fernflower I have still some problems regarding that (tried manually, when the answer in the answerfile is not present but the section exists, the answer is not loaded from the userchoices file)

@pirat89
Copy link
Member

pirat89 commented Feb 28, 2022

merging after the discussion. the issue mentioned above will be fixed in another PR as it's a different problem.

@pirat89 pirat89 merged commit 1af0007 into oamg:master Feb 28, 2022
@Rezney Rezney mentioned this pull request Mar 12, 2022
pirat89 added a commit to pirat89/leapp that referenced this pull request Mar 17, 2022
## Packaging

- Add depency on `python3` for el8+ (`python3` refers to the distribution python)
- Bump `leapp-framework` to 2.2
- Bump `leapp-framework-dependencies` to 6
- Doc: --report-schema in manpage (oamg#686)

## Framework
### Fixes

- Fix issues with initialisation of loggers (oamg#764)
- Fix TypeError during JSON serialization in dialogs on Python3 (oamg#760)
- Prevent breaking the answerfile when dialog fields contain newlines (oamg#757)
- Check answerfile upon loading (oamg#759)
- Fix the multiprocessing on Python 3.9 on Mac OS

### Enhancements

- Dialogs: print the reason field for question in the answerfile (oamg#762)
- Added possibility to specify the report format version (oamg#686)

## stdlib

### Enhancements
- Introduced `stdlib.path` library `get_common_*_path` functions for the scanning repositories, actors, etc. paths outside of the actor execution (oamg#742)
pirat89 pushed a commit to Rezney/leapp that referenced this pull request Mar 18, 2022
## Packaging

- Add depency on `python3` for el8+ (`python3` refers to the distribution python)
- Bump `leapp-framework` to 2.2
- Bump `leapp-framework-dependencies` to 6
- Doc: --report-schema in manpage (oamg#686)

## Framework
### Fixes

- Fix issues with initialisation of loggers (oamg#764)
- Fix TypeError during JSON serialization in dialogs on Python3 (oamg#760)
- Prevent breaking the answerfile when dialog fields contain newlines (oamg#757)
- Check answerfile upon loading (oamg#759)
- Fix the multiprocessing on Python 3.9 on Mac OS

### Enhancements

- Dialogs: print the reason field for question in the answerfile (oamg#762)
- Added possibility to specify the report format version (oamg#686)

## stdlib

### Enhancements
- Introduced `stdlib.path` library `get_common_*_path` functions for the scanning repositories, actors, etc. paths outside of the actor execution (oamg#742)
Rezney added a commit to Rezney/leapp that referenced this pull request Mar 18, 2022
## Packaging

- Add depency on `python3` for el8+ (`python3` refers to the distribution python)
- Bump `leapp-framework` to 2.2
- Bump `leapp-framework-dependencies` to 6
- Doc: --report-schema in manpage (oamg#686)

## Framework
### Fixes

- Fix issues with initialisation of loggers (oamg#764)
- Fix TypeError during JSON serialization in dialogs on Python3 (oamg#760)
- Prevent breaking the answerfile when dialog fields contain newlines (oamg#757)
- Check answerfile upon loading (oamg#759)
- Fix the multiprocessing on Python 3.9 on Mac OS

### Enhancements

- Dialogs: print the reason field for question in the answerfile (oamg#762)
- Added possibility to specify the report format version (oamg#686)

## stdlib

### Enhancements
- Introduced `stdlib.path` library `get_common_*_path` functions for the scanning repositories, actors, etc. paths outside of the actor execution (oamg#742)
pirat89 pushed a commit that referenced this pull request Mar 18, 2022
## Packaging

- Add depency on `python3` for el8+ (`python3` refers to the distribution python)
- Bump `leapp-framework` to 2.2
- Bump `leapp-framework-dependencies` to 6
- Doc: --report-schema in manpage (#686)

## Framework
### Fixes

- Fix issues with initialisation of loggers (#764)
- Fix TypeError during JSON serialization in dialogs on Python3 (#760)
- Prevent breaking the answerfile when dialog fields contain newlines (#757)
- Check answerfile upon loading (#759)
- Fix the multiprocessing on Python 3.9 on Mac OS

### Enhancements

- Dialogs: print the reason field for question in the answerfile (#762)
- Added possibility to specify the report format version (#686)

## stdlib

### Enhancements
- Introduced `stdlib.path` library `get_common_*_path` functions for the scanning repositories, actors, etc. paths outside of the actor execution (#742)
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.

3 participants