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

[Chore]: Investigate why the github-pr-resource is occasionally skipping versions #1719

Closed
georgethebeatle opened this issue Sep 29, 2022 · 5 comments
Assignees
Labels

Comments

@georgethebeatle
Copy link
Member

Background

We are leveraging the github-pr-resource to trigger the e2e tests on Concourse for every PR that comes in. For security reasons we are only running PRs that have the e2e-allowed label set on them. The allow-e2e task, which also watches PRs via the pr resource makes sure the test run is auto-approved if the commiter is a team member or one of our bots. Sadly, sometimes this resource skips some PRs, especially when there are many newly open PRs, like when dependabot does a few bumps. In such cases we need to manually allow e2e execution and potentially rebase or force push the change in order to trigger the tests. This is annoying and slows us down.

Action to take

Look at the github-pr-resource source code and attempt to fix the problem

Impact

Smooth PR flow

Dev Notes

No response

@georgethebeatle georgethebeatle added this to 🧊 Icebox in Korifi - Backlog via automation Sep 29, 2022
@georgethebeatle georgethebeatle moved this from 🧊 Icebox to ⚙️ Chores in Korifi - Backlog Sep 29, 2022
@danail-branekov
Copy link
Member

See telia-oss/github-pr-resource#26

Apparently this is an old issue that can occur when rebasing PR branches. The root cause seems to be that the check step of the resource is checking the CommitDate, rather than the PushDate of a commit. When a branch is rebased, the CommitDate of a commit do not change, therefore the resource thinks that it is an old one.

The resource used to be checking the PushDate but it turned out that the field is nil for PRs coming from forks. Switching to checking for CommitDate seems to be the way they addressed it.

The obvious solution of this is to check PushDate, if nil, fallback to CommitDate. This has been already suggested but it did not get much of an attention. There are multiple PRs opened to address the issue, but none of them has been actually merged.

However, there are forks of the project and one of them seem to be containing a fix we need: opendoor-labs/github-pr-resource#6

So what can we do:

  • Create yet another fork where we cherrypick a fix (or even fix it ourselves if we believe we can do that better)
  • Switch to someone else's fork and hope that they maintain it
  • Try pushing for fixing the issue (I doubt we could get much attention from the maintainers - the issue is almost 5 years old, multiple fixes/PRs have been proposed, none was actually merged)

@danail-branekov danail-branekov self-assigned this Jan 18, 2023
@danail-branekov
Copy link
Member

danail-branekov commented Jan 19, 2023

This commit's message also provides some explanation: ctreatma/github-pr-resource@c9ee589

On standup today we discussed that we need to make sure that the issue above is the culprit.

We should probably wait for the issue to occur and try debugging the check step of the resource to figure out why such PRs remain unnoticed.

blocking until the issue appears again

kieron-dev pushed a commit to cloudfoundry/korifi-ci that referenced this issue Feb 3, 2023
Add building the docker image to the ci-images pipeline and use the
resulting image in the main PR pipeline

Issue: cloudfoundry/korifi#1719
Co-authored-by: Kieron Browne <kbrowne@vmware.com>
@gcapizzi gcapizzi removed the blocked label Feb 3, 2023
@kieron-dev
Copy link
Contributor

We've decided to cherry-pick telia-oss/github-pr-resource#205 into eirini-forks. We agree with the argument that the date filtering is wrong and causes skips. We don't care about extra versions returned that concourse will filter out anyway. So it looks like a simple fix that we can maintain if upstream ever moves again.

So let's see how this goes...

@kieron-dev
Copy link
Contributor

Re-opening this as it missed 2 PRs this morning :(

@kieron-dev kieron-dev reopened this Feb 6, 2023
@kieron-dev
Copy link
Contributor

It was a pipeline problem. We set pr-label to trigger on all versions, and that will fix it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Korifi - Backlog
⚙️ Chores
Development

No branches or pull requests

4 participants