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.

Sign up for GitHub

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

Jump to bottom

Dialog creation fails for a Component without choices #826

Merged
merged 2 commits into from
May 9, 2023
Merged

Dialog creation fails for a Component without choices #826

merged 2 commits into from
May 9, 2023

Conversation

ygalblum
Copy link
Contributor

@ygalblum ygalblum commented May 1, 2023 •
edited
Loading

choices is not an attribute of Component only of some of its subclasses

@ygalblum
Dialog creation fails for a Component without choices
1b824e4 
choices is not an attribute of Component only of some its subclasses

Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
@github-actions
Copy link

github-actions bot commented May 1, 2023

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:

  • review please @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

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

  • /rerun to schedule basic regression tests using this pr build and leapp-repository*master* as artifacts
  • /rerun 42 to schedule basic regression tests using this pr build and leapp-repository*PR42* as artifacts
  • /rerun-sst to schedule sst tests using this pr build and leapp-repository*master* as artifacts
  • /rerun-sst 42 to schedule sst tests using this pr build and leapp-repository*PR42* as artifacts

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
Copy link
Contributor Author

ygalblum commented May 1, 2023

review please @oamg/developers

@pirat89
Copy link
Member

pirat89 commented May 3, 2023

@ygalblum thank you for the contribution! can you elaborate more on problems that the previous solution created? I mean, regarding the code it seems you hit some tracebacks, but I am curious what was the trigger actually as we haven't seen any so far.

@fernflower can you take a look?

@ygalblum
Copy link
Contributor Author

ygalblum commented May 3, 2023

can you elaborate more on problems that the previous solution created?

You can see the dialog I created here: https://github.com/ygalblum/leapp-supplements/blob/veritas-infoscale/repos/system_upgrade_supplements/el7toel8/actors/veritasinstallerpathfinder/actor.py#L19

Since TextComponent does not have a choices attribute, the code crashes in the line of my fix.

From a quick search I did, I couldn't find any actors that use the TextComponent and I guess it was missed in the tests.

@fernflower
Copy link
Member

Yeah, this very well might be the case as we are not using TextComponents. Could you please cover the failure with a unit test? It would really give us some peace of mind :)

@ygalblum
Copy link
Contributor Author

ygalblum commented May 4, 2023

Could you please cover the failure with a unit test?

Please see the second commit

@ygalblum
Test differnt types of Dialog components
f5e9371 
Add a multi component dialog to one of the tester actors
Verify that the input is handled correctly

Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
@pirat89 pirat89 requested a review from fernflower May 8, 2023 09:18
fernflower
fernflower approved these changes May 9, 2023
View reviewed changes
Copy link
Member

@fernflower fernflower left a comment

Choose a reason for hiding this comment

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

lgtm, thank you for the contribution!
Scheduling regression tests

@fernflower
Copy link
Member

/rerun

@github-actions
Copy link

github-actions bot commented May 9, 2023

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

@github-actions
Copy link

github-actions bot commented May 9, 2023

Testing Farm request for RHEL-8.6-rhui/5886104;5900200 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

github-actions bot commented May 9, 2023

Testing Farm request for RHEL-7.9-rhui/5886104;5900200 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

github-actions bot commented May 9, 2023

Testing Farm request for RHEL-8.6.0-Nightly/5886104;5900200 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

github-actions bot commented May 9, 2023

Testing Farm request for RHEL-8.8.0-Nightly/5886104;5900200 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@fernflower fernflower merged commit 07ff6eb into oamg:master May 9, 2023
@github-actions
Copy link

github-actions bot commented May 9, 2023

Testing Farm request for RHEL-7.9-ZStream/5886104;5900200 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

github-actions bot commented May 9, 2023

Testing Farm request for RHEL-7.9-ZStream/5886104;5900200 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@pirat89 pirat89 added this to the 7.9/8.9/9.3 milestone Jun 6, 2023
@ygalblum ygalblum deleted the text-dialog-no-choices branch June 6, 2023 12:25
pirat89 added a commit to pirat89/leapp that referenced this pull request Aug 23, 2023
@pirat89
Release 0.16.0
ef7a90c 
## Packaging
-  Provide leapp-framework 5.0 (oamg#818, oamg#840)

## Framework
### Fixes
- Improve processing of reports with UTF-8 characters (oamg#821, oamg#828)
- Fix info reporting with only one path to log (oamg#834)

### Enhancements
- Include tracebacks of actors into the leapp.db (oamg#832)

## Leapp (tool)
### Fixes
- Dialog creation fails not more for a component without choices (oamg#826)
- Empty report is generated correctly (oamg#828)
- Fix processing data in remediation instructions with non-ascii characters ()

### Enhancements
- Improve report summary output to make it more visible (oamg#818, oamg#840)

## stdlib
### Fixes
- Fixed the call when the execute cannot be performed (oamg#836)
### Enhancements
- changes related just to stdlib - under leapp/libraries/stdlib
@pirat89 pirat89 mentioned this pull request Aug 23, 2023
Merged
pirat89 added a commit to pirat89/leapp that referenced this pull request Aug 23, 2023
@pirat89
Release 0.16.0
e5ac9ae 
## Packaging
-  Provide leapp-framework 5.0 (oamg#818, oamg#840)

## Framework
### Fixes
- Improve processing of reports with UTF-8 characters (oamg#821, oamg#828)
- Fix info reporting with only one path to log (oamg#834)

### Enhancements
- Include tracebacks of actors into the leapp.db (oamg#832)

## Leapp (tool)
### Fixes
- Dialog creation fails not more for a component without choices (oamg#826)
- Empty report is generated correctly (oamg#828)
- Fix processing data in remediation instructions with non-ascii characters ()

### Enhancements
- Improve report summary output to make it more visible (oamg#818, oamg#840)

## stdlib
### Fixes
- Fixed the call when the execute cannot be performed (oamg#836)
### Enhancements
- changes related just to stdlib - under leapp/libraries/stdlib
Rezney pushed a commit that referenced this pull request Aug 23, 2023
@pirat89 @Rezney
Release 0.16.0
b57c5de 
## Packaging
-  Provide leapp-framework 5.0 (#818, #840)

## Framework
### Fixes
- Improve processing of reports with UTF-8 characters (#821, #828)
- Fix info reporting with only one path to log (#834)

### Enhancements
- Include tracebacks of actors into the leapp.db (#832)

## Leapp (tool)
### Fixes
- Dialog creation fails not more for a component without choices (#826)
- Empty report is generated correctly (#828)
- Fix processing data in remediation instructions with non-ascii characters ()

### Enhancements
- Improve report summary output to make it more visible (#818, #840)

## stdlib
### Fixes
- Fixed the call when the execute cannot be performed (#836)
### Enhancements
- changes related just to stdlib - under leapp/libraries/stdlib
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fernflower fernflower fernflower approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
7.9/8.9/9.3
Development

Successfully merging this pull request may close these issues.
3 participants
@ygalblum @pirat89 @fernflower