Enhance UpdateReadyForTesting message to be more like the others #4381
Conversation
|
Note: I sent this as a PR to get the tests run, as I cannot manage to run them locally. The virtualenv guide seems hopelessly outdated and |
AdamWill
force-pushed
the
update-ready-improve
branch
from
be8fc4b to
08a765c
Compare
|
This pull request introduces 1 alert when merging 08a765c into 2cd1d44 - view on LGTM.com new alerts:
|
AdamWill
force-pushed
the
update-ready-improve
branch
2 times, most recently
from
4d36398 to
e99af60
Compare
|
This pull request introduces 1 alert when merging e99af60 into 2cd1d44 - view on LGTM.com new alerts:
|
AdamWill
force-pushed
the
update-ready-improve
branch
from
e99af60 to
72acad9
Compare
|
On second thoughts, dropped the double inheritance. Inheriting from |
AdamWill
force-pushed
the
update-ready-improve
branch
2 times, most recently
from
b6a2b35 to
84ae08a
Compare
|
For the record, there's a bug in CI somewhere. At this point (84ae08a) the CI claims everything passed, but the messages unit tests actually failed: https://github.com/fedora-infra/bodhi/runs/5164343499?check_suite_focus=true Somehow the returncode of the test run is coming back as 0 (that's why we see |
AdamWill
force-pushed
the
update-ready-improve
branch
2 times, most recently
from
1a88984 to
d310540
Compare
|
Can you drop a note in the news directory so that this change will be listed in the release notes? I think this should be listed as backward incompatible change https://bodhi.fedoraproject.org/docs/developer/#release-notes Everything else seems fine to me. |
|
Is it backward incompatible, though? The only way I can see it being backward incompatible is if someone somehow relied on these messages not having an update dict, which seems unlikely. They still have everything they had before, they just have...more. I can do it that way if you like, just seemed a bit odd. |
|
You're right, it is not backward incompatible, my fault. Just a note to announce the new field it's ok. |
The UpdateReadyForTesting message is an odd duck. It was first made much like all the other update messages, but then changed to look completely different in response to the requirements of the CI team: https://pagure.io/fedora-ci/general/issue/70 If anyone had bothered to ask, I would've said that the QA team would quite like for the message to look just like all the others, since we'd already implemented openQA test scheduling based on the other messages, and this custom format does not include some information that is useful for that scheduling, such as whether the update is in the critical path. In an attempt to keep everyone happy, this replaces message V1 with V2, which inherits from V1 and adds the 'update' property that all the other update messages have. Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill
force-pushed
the
update-ready-improve
branch
from
d310540 to
11ef6e5
Compare
|
How's that? |
The UpdateReadyForTesting message is an odd duck. It was first
made much like all the other update messages, but then changed
to look completely different in response to the requirements of
the CI team:
https://pagure.io/fedora-ci/general/issue/70
If anyone had bothered to ask, I would've said that the QA team
would quite like for the message to look just like all the
others, since we'd already implemented openQA test scheduling
based on the other messages, and this custom format does not
include some information that is useful for that scheduling,
such as whether the update is in the critical path.
In an attempt to keep everyone happy, this replaces message V1
with V2, which inherits from V1 but also inherits from
UpdateMessage and adds the 'update' property that all the other
update messages have.
Signed-off-by: Adam Williamson awilliam@redhat.com