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

Control leapp-report format #686

Merged
merged 2 commits into from
Jan 17, 2022

Conversation

fernflower
Copy link
Member

In order to be completely safe that any changes that land in leapp
reporting don't break older clients there should be a possibility
to force specific version of json schema.
Currently there are 2 schemas that are actually backward compatible,
but it may not be the rule soon.

This patch adds a --report-schema option to leapp preupgrade/upgrade
commands that force sticking to specific version of the report schema.
For example with --report-schema '1.0.0' passed there will be no stable
keys in the report messages, as those have been added in the recent
'1.1.0' report schema.

@fernflower fernflower added the wip label Feb 9, 2021
@fernflower fernflower changed the title Control leapp-report.json format Control leapp-report format Feb 9, 2021
@centos-ci
Copy link

Can one of the admins verify this patch?

@leapp-bot
Copy link
Collaborator

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 unit tests, copr build and e2e tests in OAMG CI
  • e2e tests to run unit tests, copr build and end-to-end tests in Murphy CI (OAMG members only) [OLD PIPELINE]
  • review please to notify leapp developers of review request

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.

@pirat89
Copy link
Member

pirat89 commented Feb 10, 2021

@fernflower no review yet; just I think that the argument for the --report-schema option should be enumarated list of values. If in example we provide schema e.g. 1.5.0 in upstream, but customer will use older version of leapp (e.g. because the new one was not delivered yet), obviously such a schema will not be valid. from that point, leapp should check whether the required schema is supported or not. as well, it could happen we will have to drop completely some old schemas, because it will be completely incompatible with the new one (not searching for possible reasons, it's just possibility that could happen in future). so the enumeration/list of supported schemas should be rather present. wdyt?

@fernflower
Copy link
Member Author

fernflower commented Feb 11, 2021

@fernflower no review yet; just I think that the argument for the --report-schema option should be enumarated list of values. If in example we provide schema e.g. 1.5.0 in upstream, but customer will use older version of leapp (e.g. because the new one was not delivered yet), obviously such a schema will not be valid. from that point, leapp should check whether the required schema is supported or not. as well, it could happen we will have to drop completely some old schemas, because it will be completely incompatible with the new one (not searching for possible reasons, it's just possibility that could happen in future). so the enumeration/list of supported schemas should be rather present. wdyt?

Hmm why a list of values? My idea was that --report-schema version will mandatory force leapp to create a leapp-report.json compatible with selected version of report schema (1.0.0 without "key" and 1.1.0 with "key" for example).
If the client uses super old version of leapp he won't be able to use --report-schema cli option anyway. If leapp version is not that old but doesn't match the one in upstream (say, 1.4.0 for client and 1.5.0 for upstream) then only conversions to earlier than 1.4.0 versions are available, and the user will choose the one he wants and pass as a single string --report-schema argument.

Maybe I got you wrong and you meant that --report-schema command should output the list of possible report-schema versions to use in command description? That would make total sense.

@fernflower
Copy link
Member Author

leapp-ci build

@fernflower
Copy link
Member Author

@oamg/developers please review

@pirat89
Copy link
Member

pirat89 commented Feb 11, 2021

Passing the choices to argparse (as you did already) was the answer :)

@pirat89
Copy link
Member

pirat89 commented Feb 11, 2021

I think this could be very good candidate for our ci-hackathon, btw.

leapp/cli/upgrade/__init__.py Outdated Show resolved Hide resolved
leapp/cli/upgrade/__init__.py Outdated Show resolved Hide resolved
drehak
drehak previously approved these changes Feb 11, 2021
@drehak
Copy link
Contributor

drehak commented Feb 11, 2021

Seens good to me!

@fernflower fernflower force-pushed the reportbackwardscompatibility branch 2 times, most recently from 62e2c7f to 0cb7b73 Compare February 12, 2021 14:50
drehak
drehak previously approved these changes Feb 12, 2021
Copy link
Contributor

@drehak drehak left a 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!

@fernflower
Copy link
Member Author

@pirat89 love the idea, got rid of the separate report schema processing. Thanks!

leapp/utils/report.py Outdated Show resolved Hide resolved
pirat89
pirat89 previously approved these changes Feb 15, 2021
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.

Lgtm and works as expected. Just some english nitpicks and I think the help message is not so clear. But thiking about that, I am not sure how to re-word it to keep it short and clear.

man/leapp.1 Outdated Show resolved Hide resolved
leapp/cli/upgrade/__init__.py Outdated Show resolved Hide resolved
@pirat89 pirat89 added this to the 8.4.1 milestone Apr 17, 2021
@pirat89
Copy link
Member

pirat89 commented Apr 17, 2021

@fernflower is it still WIP? If not, I would like to merge it after the next release.

@fernflower fernflower force-pushed the reportbackwardscompatibility branch 3 times, most recently from 3e45c6a to d102f06 Compare October 5, 2021 12:49
fernflower added a commit to fernflower/leapp-repository that referenced this pull request Dec 17, 2021
Leapp-repository part of PR686 which contains appropriate changes for
the leapp preupgrade and upgrade commands.

Depends-On: oamg/leapp#686
@fernflower fernflower force-pushed the reportbackwardscompatibility branch 2 times, most recently from 9dff129 to e6837cb Compare December 17, 2021 11:33
@fernflower
Copy link
Member Author

/rerun

@github-actions
Copy link

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

@github-actions
Copy link

Testing Farm request for tmt test was created. Once finished, results should be available here.
Full pipeline log.

fernflower added a commit to fernflower/leapp-repository that referenced this pull request Dec 20, 2021
Leapp-repository part of PR686 which contains appropriate changes for
the leapp preupgrade and upgrade commands.

Depends-On: oamg/leapp#686
fernflower added a commit to fernflower/leapp-repository that referenced this pull request Jan 6, 2022
Leapp-repository part of PR686 which contains appropriate changes for
the leapp preupgrade and upgrade commands.

Depends-On: oamg/leapp#686
@fernflower fernflower force-pushed the reportbackwardscompatibility branch 2 times, most recently from 7f184ae to 524181f Compare January 7, 2022 10:57
fernflower added a commit to fernflower/leapp-repository that referenced this pull request Jan 7, 2022
Leapp-repository part of PR686 which contains appropriate changes for
the leapp preupgrade and upgrade commands.

Depends-On: oamg/leapp#686
@fernflower
Copy link
Member Author

/rerun

@github-actions
Copy link

github-actions bot commented Jan 7, 2022

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

@github-actions
Copy link

github-actions bot commented Jan 7, 2022

Testing Farm request for tmt test was created. Once finished, results should be available here.
Full pipeline log.

fernflower added a commit to fernflower/leapp-repository that referenced this pull request Jan 7, 2022
Leapp-repository part of PR686 which contains appropriate changes for
the leapp preupgrade and upgrade commands.

Depends-On: oamg/leapp#686
fernflower added a commit to fernflower/leapp-repository that referenced this pull request Jan 14, 2022
Leapp-repository part of PR686 which contains appropriate changes for
the leapp preupgrade and upgrade commands.

Depends-On: oamg/leapp#686
fernflower and others added 2 commits January 14, 2022 12:34
In order to be completely safe that any changes that land in leapp
reporting don't break older clients there should be a possibility
to force specific version of json schema.
Currently there are 2 schemas that are actually backward compatible,
but it may not be the rule soon.

This patch adds a --report-schema option to leapp preupgrade/upgrade
commands that force sticking to specific version of the report schema.
For example with --report-schema '1.0.0' passed there will be no stable
keys in the report messages, as those have been added in the recent
'1.1.0' report schema.
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 as expected and lgtm.

@pirat89 pirat89 merged commit 0fc2ab3 into oamg:master Jan 17, 2022
pirat89 pushed a commit to oamg/leapp-repository that referenced this pull request Jan 17, 2022
Leapp-repository part of PR686 which contains appropriate changes for
the leapp preupgrade and upgrade commands.

Depends-On: oamg/leapp#686
@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
Requries Repo PR Use it when leapp-deps is changed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants