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.

Sign up for GitHub

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

Jump to bottom

/packit propose-update files duplicate PRs #785

Closed
ueno opened this issue Aug 19, 2020 · 9 comments
Closed

/packit propose-update files duplicate PRs #785

ueno opened this issue Aug 19, 2020 · 9 comments
Labels
area/user-experience Usability issue

Comments

@ueno
Copy link

ueno commented Aug 19, 2020 •
edited
Loading

After triggering propose-update via issue comment, packit-service filed 3 PRs for master, f32, and f31, as expected:
p11-glue/p11-kit#314

At the same time(?) it also submitted 3 more PRs with empty content:
https://src.fedoraproject.org/rpms/p11-kit/pull-request/2
https://src.fedoraproject.org/rpms/p11-kit/pull-request/4
https://src.fedoraproject.org/rpms/p11-kit/pull-request/6

As we use a bit special packit configuration to fetch the upstream release tarball along with GPG signature, I suspect that those are triggered by "trigger: release".
@TomasTomecek TomasTomecek added the area/user-experience Usability issue label Aug 19, 2020
@TomasTomecek
Copy link
Member

TomasTomecek commented Aug 19, 2020 •
edited
Loading

You're the first who had the chance to try #761 in action. That p-s PR is retrying to perform the downstream update multiple times - that's because the release archive is usually not available when the git tags are pushed upstream (a.k.a github release is created).

I assume what happened here is that one set of PRs was created by 'trigger: release' (as you point out) and the other one using the comment.

I'm wondering how can we improve the experience so this would not happen in future for others.

(maybe track the fact that p-s is still trying and /packit propose-update would be noop and packit would respond with a comment that update is in progress)

CC @lbarcziova

@ueno
Copy link
Author

ueno commented Aug 19, 2020

(maybe track the fact that p-s is still trying and /packit propose-update would be noop and packit would respond with a comment that update is in progress)

I personally hope the other way around, i.e., 'trigger' can be optional/ignored in the configuration, as it would nevertheless fail in our use-case (because of missing GPG signature).

@TomasTomecek
Copy link
Member

Interesting, we would probably then need the data where packit-service should propose the update - I guess it could default to fedora-all but you should be able to change this with the comment posted to an issue.

@lachmanfrantisek
Copy link
Member

I personally hope the other way around, i.e., 'trigger' can be optional/ignored in the configuration, as it would nevertheless fail in our use-case (because of missing GPG signature).

So some ignore_trigger: true or manual_trigger: true option in the config and react only on comments.

Will this solve this?

@TomasTomecek do we want to have some process check as well as you described? Can we have a new issue for that?

@TomasTomecek
Copy link
Member

@lachmanfrantisek having additional options in config to handle this sounds too complicated too me

the use case is pretty clear: when you make an upstream release, p-s should propose that new release in dist-git: yes, all of this is really complicated and I'd like to hide all the complexity in p-s' codebase, ideally with the current configuration we have

I'd still suggest what I proposed originally: if update is in progress and you type /packit propose-update, p-s should respond that the update is in progress and will be tried for X times for the next Y minutes.

@TomasTomecek do we want to have some process check as well as you described? Can we have a new issue for that?

I'd like this issue to cover it.

WDYT?

@lachmanfrantisek
Copy link
Member

OK, so:

  • We don't want to support scenario where people can trigger packit-service only via comments and don't react automatically. (They can use packit CLI for that usecase.)
  • Comment for /packit propose-update when the update is in progress as you wrote.
    • What about source-not-found retrigger -- do we count that as a progress?

@TomasTomecek
Copy link
Member 

* We don't want to support scenario where people can trigger packit-service only via comments and don't react automatically. (They can use packit CLI for that usecase.)

+1, exactly

* Comment for `/packit propose-update` when the update is in progress as you wrote.
  
  * What about source-not-found retrigger -- do we count that as a progress?

yup, that's what I had in mind

WDYT?

@lachmanfrantisek lachmanfrantisek mentioned this issue Oct 5, 2020
Open
@lbarcziova lbarcziova mentioned this issue Oct 19, 2020
Merged
1 task
@lachmanfrantisek
Copy link
Member

Can we close this since the problem of creating multiple PRs was fixed in packit/packit#994?

This missing part is about response for triggering of the already running job. Do we want to solve it here or create a separate issue?

@lbarcziova
Copy link
Member

I am for closing this and creating a separate issue for that.

@lachmanfrantisek lachmanfrantisek mentioned this issue Nov 2, 2020
Closed
2 tasks
@lachmanfrantisek lachmanfrantisek mentioned this issue Nov 12, 2020
Closed
@jpopelka jpopelka mentioned this issue Sep 14, 2022
Merged
jpopelka added a commit to jpopelka/packit that referenced this issue Sep 14, 2022
@jpopelka
Push to fork before checking for existing PR
38a499a 
Previously, when doing propose downstream, we checked for
an already existing PR, and only if there was none, we pushed changes
to a fork and created a new PR.
Initially, the checking for already existing PR had been added in packit#994
in order to prevent creating multiple PRs if more propose-downstream
jobs are triggered (packit/packit-service#785).

This prevents us from updating an already existing PR (packit/hardly#76),
so with this change, we first push to a fork,
which might update an already existing PR,
and then we check whether we actually need to create a new PR
or if there already is one.
jpopelka added a commit to jpopelka/packit that referenced this issue Sep 15, 2022
@jpopelka
Push to fork before checking for existing PR
ca72b4c 
Previously, when doing propose downstream, we checked for
an already existing PR, and only if there was none, we pushed changes
to a fork and created a new PR.
Initially, the checking for already existing PR had been added in packit#994
in order to prevent creating multiple PRs if more propose-downstream
jobs are triggered (packit/packit-service#785).

This prevents us from updating an already existing PR (packit/hardly#76),
so with this change, we first push to a fork,
which might update an already existing PR,
and then we check whether we actually need to create a new PR
or if there already is one.
jpopelka added a commit to jpopelka/packit that referenced this issue Sep 19, 2022
@jpopelka
Push to fork before checking for existing PR
6f71166 
Previously, when doing propose downstream, we checked for
an already existing PR, and only if there was none, we pushed changes
to a fork and created a new PR.
Initially, the checking for already existing PR had been added in packit#994
in order to prevent creating multiple PRs if more propose-downstream
jobs are triggered (packit/packit-service#785).

This prevents us from updating an already existing PR (packit/hardly#76),
so with this change, we first push to a fork,
which might update an already existing PR,
and then we check whether we actually need to create a new PR
or if there already is one.
jpopelka added a commit to jpopelka/packit that referenced this issue Sep 19, 2022
@jpopelka
Push to fork before checking for existing PR
e74c47b 
Previously, when doing propose downstream, we checked for
an already existing PR, and only if there was none, we pushed changes
to a fork and created a new PR.
Initially, the checking for already existing PR had been added in packit#994
in order to prevent creating multiple PRs if more propose-downstream
jobs are triggered (packit/packit-service#785).

This prevents us from updating an already existing PR (packit/hardly#76),
so with this change, we first push to a fork,
which might update an already existing PR,
and then we check whether we actually need to create a new PR
or if there already is one.
softwarefactory-project-zuul bot added a commit to packit/packit that referenced this issue Sep 19, 2022
@softwarefactory-project-zuul
Merge pull request #1725 from jpopelka/sync_release_update_pr
dbb72a7 
Push to fork before checking for existing PR

Previously, when doing propose downstream, we checked for an already existing PR, and only if there was none, we pushed changes to a fork and created a new PR. Initially, the checking for already existing PR had been added in #994 in order to prevent creating multiple PRs if more propose-downstream jobs are triggered (packit/packit-service#785).
This prevents us from updating an already existing PR (packit/hardly#76), so with this change, we first push to a fork, which might update an already existing PR, and then we check whether we actually need to create a new PR or if there already is one.
In #994 (comment) @lachmanfrantisek thought that the change already worked this way (i.e. that the push is done before the check), until he realized it's not :-)
But he expressed his liking of it, so I hope it's still OK :-)
Related to packit/hardly#76
RELEASE NOTES BEGIN
Propose downstream job now pushes changes even when it's not creating a new pull request. This allows updating already existing pull requests.
RELEASE NOTES END

Reviewed-by: Laura Barcziová <None>
Reviewed-by: None <None>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
area/user-experience Usability issue
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@ueno @TomasTomecek @lachmanfrantisek @lbarcziova