Parse the highest from newly retrieved upstream version#3024
Parse the highest from newly retrieved upstream version#3024nforro wants to merge 1 commit intopackit:mainfrom
Conversation
|
Merge Failed. This change or one of its cross-repo dependencies was unable to be automatically merged with the current state of its repository. Please rebase the change and upload a new patchset. |
There was a problem hiding this comment.
Code Review
This pull request correctly changes the logic to parse the highest version from the upstream_versions field in the new hotness update event, which is more accurate than using the top-level version field. I've added one suggestion to make the implementation more robust against cases where upstream_versions might be missing or empty in the event payload, which would prevent potential runtime errors.
JoseExposito
left a comment
There was a problem hiding this comment.
Hey Nikola,
Thanks a lot for implementing this feature!!
I added a minor comment about the version path in the JSON, but other than that (and with no context of the rest of the code of the project) this change looks good to me.
b5c2ff0 to
3d79942
Compare
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 47s |
lbarcziova
left a comment
There was a problem hiding this comment.
the change looks good to me, is there any chance this could break existing users? @mfocko could you please also have a look, I recall you were dealing with this code for CentOS stream updates.
I don't think so, but I can't be 100% sure.
Weren't those Anitya messages? |
Some projects have multiple release streams and the latest version released upstream may not be the highest version ever captured. The `version` field always contains the highest recorded version, no matter what version actually triggered the event. Instead, read the version from the `upstream_versions` field that contains newly retrieved versions. Unfortunately, there can be more of those (depending on the configured backend), so choose the highest one. Signed-off-by: Nikola Forró <nforro@redhat.com>
3d79942 to
7aee1a8
Compare
I mean, this could only break projects where the latest upstream version is not always the highest, but that can be mitigated with |
|
Build succeeded. ✔️ pre-commit SUCCESS in 1m 49s |
If I understood correctly, monitoring status would need to be set to "monitoring all" as well. The recommended option in the docs is "monitoring". The former notifies about all releases, the later notifies only about the highest. So that could be another way of mitigation/configuration. |
Good point 👍 |
|
Am I right that this will not work in case upstream releases two versions at the same time? Which I think is not a rare case, for example when upstream issues a fix for CVE across multiple supported versions of the same project. |
That's still two releases, hence two separate events. |
Actually, thinking about it, it probably depends on the backend and how (and how often) it parses the releases. If that's the case, we need to handle this on Packit side by cloning the event and setting proper version in each instance. |
|
@nforro That was the reason why the Just to show some example: Upstream project releases version 2.5.1 and also release version 1.9.2 with the same fix. In current state |
After giving this more thought, it would be better to capture the list of versions in the event (like |
| from packit.config import JobConfigTriggerType | ||
| from packit.constants import DISTGIT_INSTANCES | ||
| from packit.utils import nested_get | ||
| from packit.utils.versions import compare_versions |
There was a problem hiding this comment.
Wouldn't it make sense to move this, sometime in the future, to the specfile?
There was a problem hiding this comment.
Perhaps, though this is targeted to compare Python version specifiers (compatible with SemVer), while specfile already provides comparable (N)EVR(A) objects that represent RPM versioning with its rules.
There was a problem hiding this comment.
I see… sorry for opening another can of worms…
@lbarcziova CentOS should consume different topic, Anitya, IIRC, produces either
|
|
@nforro if we don't want to support multi-version sync from the CLI , then I agree that extending the handler to loop over the versions and create a separate sync run per version could be more straightforward. This could help with easier reporting/dashboard separation. Since the previous work was really initial, I think starting fresh at the handler level could be the cleaner path if we all agree. |
|
Thanks, let's discuss this tomorrow on arch. To reiterate, the plan would be:
Implications:
|
Some projects have multiple release streams and the latest version released upstream may not be the highest version ever captured. The
versionfield always contains the highest recorded version, no matter what version actually triggered the event. Instead, read the version from theupstream_versionsfield that contains newly retrieved versions. Unfortunately, there can be more of those (depending on the configured backend), so choose the highest one.For more context see fedora-infra/anitya#2009.