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.

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

PR title in the queue doesn't get updated when PR title changes in the repo #51

Closed
japaric opened this issue Jan 28, 2015 · 12 comments
Closed

Comments

@japaric
Copy link

japaric commented Jan 28, 2015

For example rust-lang/rust#21677 title was changed to "snapshot + s/range(a, b)/a..b/g + s/Show/Debug/g cleanup", but it still shows up as "cleanup: s/range(a, b)/a..b/g" in the queue.

@sanxiyn
Copy link

sanxiyn commented Jan 29, 2015

As I understand, this is a limitation of GitHub API in that there is no way to receive notifications when pull requests are retitled.

@japaric
Copy link
Author

japaric commented Jan 29, 2015

@sanxiyn even if you don't get notifications about changes of the PR title, you could still update the title every time you get a notification for any other reason.

@barosl
Copy link
Owner

barosl commented Jan 31, 2015

This bothered me too, but I've believed that it is impossible to fix due to the shortcomings of the GitHub API. I'm glad to know there's a workaround. Thanks for informing me, @japaric!

@pnkfelix
Copy link

Fixing this would be nice; one of my entries in the current queue is prefixed with "no checkin", because when I first posted the PR, I wanted to make sure it would not be checked in. (And then I updated the title when it was ready to be checked in)

But now I know that I cannot actually trust such meta info in the titles that homu presents, at least not until this bug is fixed.

@barosl
Copy link
Owner

barosl commented Feb 11, 2015

This issue was fixed in my local branch, but I couldn't push it to master yet as I've been traveling nowadays. I will try to resolve this tomorrow. Sorry for the inconvenience!

@pnkfelix
Copy link

@barosl ah great.

Sorry if my earlier comment sounded super-whiny; I was more kicking myself for not just opening a fresh PR in the first place, since I had been considering doing so.

@barosl barosl closed this as completed in e9314ca Feb 22, 2015
@pnkfelix
Copy link

When this was fixed, does it also update the associated description (which may also have been updated? I just noticed a [breaking-change] note missing from

rust-lang/rust@b08d6cf

Even though it was present in rust-lang/rust#24500 at the time of the merge itself.

(If that was part of the fix ... then have we really not upgraded homu since Feb 22nd???)

@barosl
Copy link
Owner

barosl commented Apr 21, 2015

@pnkfelix The title and the description are updated when an additional comment is posted. Maybe you didn't comment on the issue after you modified the description?

@pnkfelix
Copy link

@barosl doesn't bors itself post comments (namely, the statuses on the PR)? Or ... I guess does it treat the comments from bors as a special case? (I don't yet see why that would be necessary, I don't think it would cause an infinite loop, but maybe I am overlooking something...)

Anyway, would it be too much to have homu do a final read of the title and description at the time of the merge, for the purposes of deciding what should actually go into the merge commit? Should open a separate issue for this?

@barosl
Copy link
Owner

barosl commented Apr 21, 2015

@pnkfelix The metadata does get updated when Homu posts a comment, but at the time of reporting the status, the merge commit is already made, so it cannot utilize the updated metadata... (If you retry the PR, it will use the updated one)

I thought of the "final read" approach, but at that time I wanted to minimize the API usage as much as possible. However, thinking again, one API usage per merging doesn't seem to consume much. So I think a separate issue can be opened.

@pnkfelix
Copy link

@barosl okay great, thanks

@pnkfelix
Copy link

@barosl p.s. I had thought I had updated the description at the time that Homu posted this comment which predates the merge by a day ... but it certainly is possible that I am misremembering the order of events. (It is unfortunate that github does not record timestamps for comment edits.)

Manishearth pushed a commit to Manishearth/homu that referenced this issue Jul 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants