Add a stable report identifier #669

fernflower · 2020-10-06T09:22:31Z

A 'key' field will be added to each report entry. Same report entries on different machines should have the same 'key' value.

From the user/contributor perspective the interface doesn't change, if the Key with the unique report key is not defined in the report entries passed to the create_report command, the framework will take care of stable identifiers by generating one from the required report fields or will raise a ValueError in case LEAPP_DEVEL_FIXED_REPORT_KEY environment
variable is defined.
This fallback behavior will be the last resort once all actors in the leapp-repository are updated.

centos-ci · 2020-10-06T09:23:02Z

Can one of the admins verify this patch?

leapp-bot · 2020-10-06T09:23:59Z

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.

zhukovgreen

Just a few comments

leapp/reporting/__init__.py

zhukovgreen

Just some replies

leapp/reporting/__init__.py

pirat89 · 2020-10-19T09:47:58Z

@fernflower I am not sure whether I get it, but if you plan to provide keys for reports from leapp-repository via a file in leapp, it's no go.

leapp/reporting/__init__.py

pirat89 · 2020-10-20T12:58:51Z

@fernflower btw, remember to bump leapp-framework in the PR (and if another following PR is in leapp-repository, the leapp-framework requested values should be bumped there as well). See this doc for more info: https://leapp.readthedocs.io/en/latest/compatibility-with-leapp-repository.html

zhukovgreen · 2020-11-06T09:26:53Z

@fernflower Hi Inna, will you add a test case when the key wasn't provided inside the report?

fernflower · 2020-11-06T09:30:54Z

@fernflower Hi Inna, will you add a test case when the key wasn't provided inside the report?

Already there in all flavors I could think of https://github.com/oamg/leapp/pull/669/files#diff-298ab46eb87d51b2b477ad88e9730c63ea0885d7f797933de81e9a246b96c8f1

Did I miss some testcase?

zhukovgreen · 2020-11-06T09:45:02Z

@fernflower already discussed that on IRC. Thx

zhukovgreen

Adding a question which might affect the implementation gradually
/cc @fernflower @pirat89

leapp/reporting/__init__.py

shaded-enmity

I think we're mostly good to go, however what is not really clear to me is how do I set an ID for a dialog? i.e. add key field to the DialogModel? https://github.com/oamg/leapp/blob/master/leapp/models/__init__.py#L153 so that verifydialogs in leapp-repository can either forward what is in the model or fallback to autogeneration.

fernflower · 2020-11-10T19:34:32Z

I think we're mostly good to go, however what is not really clear to me is how do I set an ID for a dialog? i.e. add key field to the DialogModel? https://github.com/oamg/leapp/blob/master/leapp/models/__init__.py#L153 so that verifydialogs in leapp-repository can either forward what is in the model or fallback to autogeneration.

Great point, yeah, adding a key into the Dialog class and DialogModel should do this.

To be honest now I am questioning if we need special "Dialog key auto generation" introduced in the commit cc727f0? Maybe it is cleaner to go with "by default all dialogs missing answers will have the same key generated by default" and then in the parallel leapp-repository patch specifically use a dialog.key in the create_report https://github.com/oamg/leapp-repository/blob/master/repos/system_upgrade/el7toel8/actors/verifydialogs/libraries/verifydialogs.py#L23? I am really not in favor of this "autogenerate the dialog stable report key in the framework by utilizing the implementation details of RelatedResource(sectionX) being present among the report entries" approach I had to use :(

zhukovgreen

Looks really good to me!

For test_create_report_stable_key I would go with the pytest.parametrize, as the current impl seems a bit bulky, but this is really a matter of taste.

shaded-enmity · 2020-11-11T13:34:09Z

+1 that the current tight coupling between leapp and leapp-repository via verifydialogs is a bit unwieldy (well, more than a bit I guess), but honestly I have no idea how to break the cycle. What I think could work:

Introduce a new Phase flag, like ReportCollection
Move verifydialogs somewhere to the framework
Run verifydialogs actor in the ReportCollection flagged phase
(maybe) ensure no other actors are run in the phase

This way we still retain the flexibility that workflows can decide when to collect+verify reports by flagging the particular phase with the ReportCollection flag (or skip completely if there's no phase with that flag).

Same ID for all dialog-based reports makes me really nervous, I can see people automating based on IDs and waiving more dialogs than they expect.

fernflower · 2020-11-11T13:38:18Z

Same ID for all dialog-based reports makes me really nervous, I can see people automating based on IDs and waiving more dialogs than they expect.

No-no, I meant temporary (the "stable report key" is a feature that is not used anyway yet) until a parallel patch lands into leapp-repository for verifydialogs actor, smth like

--- a/repos/system_upgrade/el7toel8/actors/verifydialogs/libraries/verifydialogs.py
+++ b/repos/system_upgrade/el7toel8/actors/verifydialogs/libraries/verifydialogs.py
@@ -19,5 +19,6 @@ def check_dialogs(inhibit_if_no_userchoice=True):
                        reporting.Severity(reporting.Severity.HIGH),
                        reporting.Summary(summary.format('\n'.join(sections))),
                        reporting.Flags([reporting.Flags.INHIBITOR] if inhibit_if_no_userchoice else []),
-                       reporting.Remediation(hint=dialogs_remediation, commands=cmd_remediation)]
+                       reporting.Remediation(hint=dialogs_remediation, commands=cmd_remediation),
+                       reporting.Key(dialog.key)]

Depends-On: oamg/leapp#669

A 'key' field will be added to each report entry. Same report entries on different machines should have the same 'key' value. From the user/contributor perspective the interface doesn't change, the framework will take care of stable identifiers' dynamic generation if the user doesn't set it.

Depends-On: oamg/leapp#669

fernflower · 2021-01-11T14:36:48Z

@AloisMahdal That's a fantastic example of the importance of cross-repo testing - the failure you discovered with the leapp-repository patch is actually caused by the leapp bug.
Should be fixed with the latest commit, once the build is there 2 patches can be retested together.

By the way the upgrade was never continued, it broke pretty bad but didn't run without the filled-in answerfile.

AloisMahdal · 2021-01-12T09:44:21Z

Fixed: as of

leapp-0.11.0-0.202101111435Z.04c5ef0.stablereportid.PR669.el7_8
leapp-repository-0.12.0-0.202101081216Z.5cf1bc4.stablereport.PR618.el7_8

the upgrade is inhibited properly:

Risk Factor: high (inhibitor)
Title: Missing required answers in the answer file
Summary: One or more sections in answerfile are missing user choices: remove_pam_pkcs11_module_check.confirm
For more information consult https://leapp.readthedocs.io/en/latest/dialogs.html
Remediation: [hint] Please register user choices with leapp answer cli command or by manually editing the answerfile.
[command] leapp answer --section remove_pam_pkcs11_module_check.confirm=True
Key: d35f6c6b1b1fa6924ef442e3670d90fa92f0d54b
----------------------------------------
Risk Factor: high
Title: Upgrade is unsupported
Summary: Environment variable LEAPP_UNSUPPORTED has been detected. A successful and safe upgrade process cannot be guaranteed. From now on you are continuing at your own risk.

Key: 9e5088e3c1f371e020ec777c3d86578f4be143cf
----------------------------------------
Risk Factor: high
Title: Difference in Python versions and support in RHEL 8
Summary: In RHEL 8, there is no 'python' command. Python 3 (backward incompatible) is the primary Python version and Python 2 is available with limited support and limited set of packages. Read more here: https://access.redhat.com/documentation/en-us/red_hat_enterprise_linux/8/html-single/configuring_basic_system_settings/#using-python3
Remediation: [hint] Please run "alternatives --set python /usr/bin/python3" after upgrade
Key: 0c98585b1d8d252eb540bf61560094f3495351f5
    [...]
    [...]
    [...]

Thanks!

pirat89

Lgtm and it seems it works as expected.

pirat89 · 2021-01-12T18:31:51Z

@fernflower you can do it in separate PR, but pls update the doc abou tthe reporting to reflect the possibility to add Key - including how people should generate the key.

AloisMahdal · 2021-01-13T08:28:47Z

latest murphy-ci/* failures are caused by internal bug (my bad -- sorry) which is fixed now; re-running

AloisMahdal · 2021-01-13T12:02:07Z

murphy-ci/integration - I have confirmed that the fail can be waived safely.

i.e.: from QE POV, this PR is good to go.

Depends-On: oamg/leapp#669

fernflower · 2021-01-25T09:29:26Z

leapp-ci build

fernflower · 2021-01-27T22:38:47Z

leapp-ci build

pirat89 · 2021-01-28T08:29:03Z

leapp-ci build

This final touch to stable id generation will make reports from the same actor but with different dialogs have different ids. Depends-On: oamg/leapp#669

pirat89 · 2021-01-28T10:33:35Z

just for info, current implementation includes the severity field when the default key is generated, but in case of the spamassassinconfigcheck actor in the official leapp-repository repo, the severity is variable - which is valid case - and I would expect really stable ID. In this particular case it can be easily fixed defining the particular stable id in the report. but just pointing the fact the severity is expected right now to be variable in some cases and from my pov it still about only one specific report.

pirat89 · 2021-01-28T11:12:44Z

leapp-ci build

fernflower · 2021-01-28T12:51:10Z

e2e tests

examon · 2021-01-28T15:33:06Z

e2e tests

This final touch to stable id generation will make reports from the same actor but with different dialogs have different ids. Depends-On: oamg/leapp#669

pirat89 · 2021-01-29T13:05:07Z

@fernflower 🥳

This final touch to stable id generation will make reports from the same actor but with different dialogs have different ids. Depends-On: oamg/leapp#669

## Packaging - Bump leapp-framework capability to 1.4 ## Framework ### Fixes - Replace functools.wraps with six.wraps (oamg#674) ### Enhancements - Add a stable report identifier (oamg#669) - Add a way to override default python version through environment variables (oamg#675) - Add a possibility to overwrite virtualenv name through environment variables (oamg#675) Related leapp-repository release: https://github.com/oamg/leapp-repository/releases/tag/v0.13.0

## Packaging - Bump leapp-framework capability to 1.4 ## Framework ### Fixes - Replace functools.wraps with six.wraps (oamg#674) ### Enhancements - Add a stable report identifier (oamg#669) - Add a way to override default python version through environment variables (oamg#675) - Add a possibility to overwrite virtualenv name through environment variables (oamg#675) Related leapp-repository release: https://github.com/oamg/leapp-repository/releases/tag/v0.13.0 # Please enter the commit message for your changes. Lines starting # with '#' will be kept; you may remove them yourself if you want to. # An empty message aborts the commit. # # Date: Tue Feb 2 15:57:58 2021 +0100 # # On branch release/202102 # Your branch is up to date with 'drehak/release/202102'. # # No changes

## Packaging - Add JSON schema of leapp reports for validation (oamg#681) - Bump leapp-framework capability to 1.4 (oamg#684) ## Framework ### Fixes - Fix Py2/Py3 issues for unit-tests relying on __wrapped__ for decorators (oamg#674) ### Enhancements - Add a stable report identifier for each generated report (oamg#669) ## Additional changes interesting for devels - Add a possibility to overwrite virtualenv name for testing `$VENVNAME` (oamg#675) - Add a way to override default python version using `$PYTHON_VENV` (oamg#675) Related leapp-repository release: https://github.com/oamg/leapp-repository/releases/tag/v0.13.0

## Packaging - Add JSON schema of leapp reports for validation (#681) - Bump leapp-framework capability to 1.4 (#684) ## Framework ### Fixes - Fix Py2/Py3 issues for unit-tests relying on __wrapped__ for decorators (#674) ### Enhancements - Add a stable report identifier for each generated report (#669) ## Additional changes interesting for devels - Add a possibility to overwrite virtualenv name for testing `$VENVNAME` (#675) - Add a way to override default python version using `$PYTHON_VENV` (#675) Related leapp-repository release: https://github.com/oamg/leapp-repository/releases/tag/v0.13.0

fernflower added the wip label Oct 6, 2020

fernflower force-pushed the stablereportid branch from c4f5257 to d532a49 Compare October 6, 2020 10:38

zhukovgreen reviewed Oct 8, 2020

View reviewed changes

leapp/reporting/__init__.py Outdated Show resolved Hide resolved

leapp/reporting/__init__.py Outdated Show resolved Hide resolved

leapp/reporting/__init__.py Show resolved Hide resolved

fernflower requested review from Rezney, pirat89 and shaded-enmity October 19, 2020 07:31

zhukovgreen reviewed Oct 19, 2020

View reviewed changes

leapp/reporting/__init__.py Outdated Show resolved Hide resolved

pirat89 reviewed Oct 19, 2020

View reviewed changes

leapp/reporting/__init__.py Outdated Show resolved Hide resolved

zhukovgreen reviewed Nov 6, 2020

View reviewed changes

leapp/reporting/__init__.py Outdated Show resolved Hide resolved

fernflower removed the wip label Nov 9, 2020

shaded-enmity reviewed Nov 10, 2020

View reviewed changes

fernflower requested review from shaded-enmity, zhukovgreen and pirat89 November 11, 2020 10:29

zhukovgreen previously approved these changes Nov 11, 2020

View reviewed changes

fernflower added a commit to fernflower/leapp-repository that referenced this pull request Nov 11, 2020

verifydialogs: Pass a unique Key to the create_report

0e4cf66

Depends-On: oamg/leapp#669

fernflower mentioned this pull request Nov 11, 2020

verifydialogs: Pass a unique Key to the create_report oamg/leapp-repository#618

Merged

fernflower dismissed zhukovgreen’s stale review via 14d688b November 11, 2020 14:21

fernflower force-pushed the stablereportid branch from 296765e to ce2ae08 Compare November 19, 2020 12:15

fernflower added a commit to fernflower/leapp-repository that referenced this pull request Jan 8, 2021

verifydialogs: Pass a unique Key to the create_report

75a671a

Depends-On: oamg/leapp#669

fernflower added 2 commits January 11, 2021 15:20

fix allowed key check for unicode and str

f999c24

Check allowed uuid type for key with six.string_types

04c5ef0

pirat89 approved these changes Jan 12, 2021

View reviewed changes

fernflower added a commit to fernflower/leapp-repository that referenced this pull request Jan 13, 2021

verifydialogs: Pass a unique Key to the create_report

2167c62

Depends-On: oamg/leapp#669

pirat89 merged commit 4e8a87f into oamg:master Jan 29, 2021

drehak mentioned this pull request Feb 2, 2021

Release 0.12.0 #685

Merged

MichalHe pushed a commit to MichalHe/leapp that referenced this pull request Aug 12, 2021

Add workflow that triggers copr builds for master branch (oamg#669)

c092f03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a stable report identifier #669

Add a stable report identifier #669

fernflower commented Oct 6, 2020 •

edited

centos-ci commented Oct 6, 2020

leapp-bot commented Oct 6, 2020

zhukovgreen left a comment

zhukovgreen left a comment

pirat89 commented Oct 19, 2020

pirat89 commented Oct 20, 2020 •

edited

zhukovgreen commented Nov 6, 2020

fernflower commented Nov 6, 2020

zhukovgreen commented Nov 6, 2020

zhukovgreen left a comment

shaded-enmity left a comment

fernflower commented Nov 10, 2020 •

edited

zhukovgreen left a comment

shaded-enmity commented Nov 11, 2020

fernflower commented Nov 11, 2020

fernflower commented Jan 11, 2021

AloisMahdal commented Jan 12, 2021

pirat89 left a comment

pirat89 commented Jan 12, 2021

AloisMahdal commented Jan 13, 2021

AloisMahdal commented Jan 13, 2021

fernflower commented Jan 25, 2021

fernflower commented Jan 27, 2021

pirat89 commented Jan 28, 2021

pirat89 commented Jan 28, 2021

pirat89 commented Jan 28, 2021

fernflower commented Jan 28, 2021

examon commented Jan 28, 2021

pirat89 commented Jan 29, 2021

Add a stable report identifier #669

Add a stable report identifier #669

Conversation

fernflower commented Oct 6, 2020 • edited

centos-ci commented Oct 6, 2020

leapp-bot commented Oct 6, 2020

Thank you for contributing to the Leapp project!

zhukovgreen left a comment

Choose a reason for hiding this comment

zhukovgreen left a comment

Choose a reason for hiding this comment

pirat89 commented Oct 19, 2020

pirat89 commented Oct 20, 2020 • edited

zhukovgreen commented Nov 6, 2020

fernflower commented Nov 6, 2020

zhukovgreen commented Nov 6, 2020

zhukovgreen left a comment

Choose a reason for hiding this comment

shaded-enmity left a comment

Choose a reason for hiding this comment

fernflower commented Nov 10, 2020 • edited

zhukovgreen left a comment

Choose a reason for hiding this comment

shaded-enmity commented Nov 11, 2020

fernflower commented Nov 11, 2020

fernflower commented Jan 11, 2021

AloisMahdal commented Jan 12, 2021

pirat89 left a comment

Choose a reason for hiding this comment

pirat89 commented Jan 12, 2021

AloisMahdal commented Jan 13, 2021

AloisMahdal commented Jan 13, 2021

fernflower commented Jan 25, 2021

fernflower commented Jan 27, 2021

pirat89 commented Jan 28, 2021

pirat89 commented Jan 28, 2021

pirat89 commented Jan 28, 2021

fernflower commented Jan 28, 2021

examon commented Jan 28, 2021

pirat89 commented Jan 29, 2021

fernflower commented Oct 6, 2020 •

edited

pirat89 commented Oct 20, 2020 •

edited

fernflower commented Nov 10, 2020 •

edited