-
Notifications
You must be signed in to change notification settings - Fork 83
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
Allow passing versions as list for sync_release #2317
Allow passing versions as list for sync_release #2317
Conversation
Build failed. ✔️ pre-commit SUCCESS in 2m 12s |
b7ecdfc
to
feb6e83
Compare
Build succeeded. ✔️ pre-commit SUCCESS in 2m 02s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have a feeling that having both version
and versions
will introduce some bugs, WDYT
@@ -771,14 +771,12 @@ def check_version_distance( | |||
and masked_proposed | |||
and masked_current.group(0) != masked_proposed.group(0) | |||
): | |||
raise ReleaseSkippedPackitException( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:classic: Pythonic… instead of having a predicate, yeet the error out /o
packit/api.py
Outdated
version: Optional[str] = None, | ||
tag: Optional[str] = None, | ||
versions: Optional[list[str]] = None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial idea I had, was to remove (or at least deprecate) the version
and use just versions
, since it doesn't really matter whether there is just one version in the list or multiple 🤔
Now that I'm thinking about it, we could even have version as a local variable, start by populating it (from nothing, the if
s you have expanded to check for versions
), and then just use as we did before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to be rather explicit here whether there is one concrete version to propose or it should be determined in the method later (this cannot happen right in the beginning since we use the checked-out branch to get the current version in specfile for comparisons); so that way it seemed to me like less intrusive
also, in packit-service, we currently utilise the tag
rather than version
, see here, so the code for anitya will need to be still changed
but I am not strongly against, so I can rework it that way if you prefer that solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reworked, PTAL 🙏
feb6e83
to
3206fd6
Compare
Build succeeded. ✔️ pre-commit SUCCESS in 4m 23s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since it changes the current API, do you have a follow up in packit-service open? 👀 I haven't seen any references in this PR.
LGTM
as I wrote in the comments above, in the service we are actually passing |
2071028
to
415f98c
Compare
Build succeeded. ✔️ pre-commit SUCCESS in 3m 50s |
Build succeeded (gate pipeline). ✔️ pre-commit SUCCESS in 2m 05s |
c8c0314
into
packit:main
Fixes packit/packit-service#2408
Related to packit/packit-service#2068
Allow passing it as a new parameter to be explicit. With this, the CLI/service doesn't need to be touched for now and in followup implementation (next points in packit/packit-service#2068) the list will be passed only for
AnityaVersionUpdateEvent
.RELEASE NOTES BEGIN
N/A
RELEASE NOTES END