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

Track commit push time #1233

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Track commit push time #1233

merged 1 commit into from
Nov 30, 2023

Conversation

TimoWilken
Copy link
Contributor

This should give a good approximation of the time a commit was submitted for testing. It still breaks down for draft PRs that are marked non-draft after a while.

Draft because it needs testing.

@TimoWilken TimoWilken force-pushed the track-pr-delay branch 2 times, most recently from 0b4b1df to bc5acf2 Compare November 29, 2023 13:31
@TimoWilken TimoWilken marked this pull request as ready for review November 29, 2023 16:20
@TimoWilken TimoWilken requested a review from ktf as a code owner November 29, 2023 16:20
@TimoWilken
Copy link
Contributor Author

Tested and working in CI now @ktf; ready for review!

@ktf
Copy link
Member

ktf commented Nov 30, 2023

Can you squash the commits and provide a summary? You mention some issue with Drafts PRs but then it's not clear from the history if this has been solved.

This should give a good approximation of the time a commit was submitted for
testing. It still breaks down for draft PRs that are marked non-draft after a
while, but PRs that are opened as non-draft from old commits are handled
correctly.

The alternative would be to use the PR's updatedAt property, but then any
comments posted in the PR would reset the waiting timer.
@TimoWilken
Copy link
Contributor Author

Squashed. Draft PRs are unfortunately still a problem, but I can't see a way around it using the GraphQL API.

The alternative to the current approach would be to use the PR's updatedAt property, but then any comments posted in the PR would reset the waiting timer.

@ktf
Copy link
Member

ktf commented Nov 30, 2023

As long as the only side effect is that draft PRs will not have the correct number associated, I think we do not care.

@TimoWilken
Copy link
Contributor Author

Yes, the problem only occurs for PRs that were draft at the time of their latest push (or creation) and are then made non-draft later. In that case, I'll merge this PR, since I've tested the changes already.

@TimoWilken TimoWilken merged commit 6135d6d into alisw:master Nov 30, 2023
1 check passed
@TimoWilken TimoWilken deleted the track-pr-delay branch November 30, 2023 13:02
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

Successfully merging this pull request may close these issues.

2 participants