Skip to content

GitHub checks on deploy-* PRs generate the wrong link #241

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

Closed
krivard opened this issue Aug 24, 2020 · 4 comments
Closed

GitHub checks on deploy-* PRs generate the wrong link #241

krivard opened this issue Aug 24, 2020 · 4 comments
Assignees

Comments

@krivard
Copy link
Contributor

krivard commented Aug 24, 2020

The “Details” link on PR 230 actually takes you to the Jenkins console for PR 239

@sgratzl
Copy link
Member

sgratzl commented Aug 24, 2020

I don't think that anything is wrong. It is just a weird cornercase:

PR239, PR230, and PR235 want to merge the same last commit into different branches, namely: 13ad138

However, there can only be one commit check of one source at a time. In this case the latest on wins: the jenkins run of PR239

@krivard
Copy link
Contributor Author

krivard commented Aug 24, 2020

The failed check for PR230 does exist -- are you saying that GitHub overwrote that check reference with the PR239 check reference, just because the later PR included one of the same commits? That seems bizarre.

The behavior we want is for a PR against deploy-X to run the commit checks for indicator X, and no other checks. I know that ideally, each indicator would live in its own codebase, so we wouldn't have to deal with commits to indicator Y in a PR that's about indicator X. However, that would eventually clutter the cmu-delphi organization with dozens of indicator codebases, and that seemed like a bad idea. If however GitHub insists on letting a commit check bleed over into PRs to which that check does not apply, we may have to reorganize things substantially.

@sgratzl
Copy link
Member

sgratzl commented Aug 25, 2020

as far as I understood checks run against commits not PRs per se. They are just identified by CIs (like Jenkins) as the last commit of the PR that is going to be merged. In this case all of the three PRs have the same commit as their last one: 13ad138. Since there can only be one check for each commit, the last check (by PR 239) overwrote the checks by the other ones. This works well in most git branch setups since you usually merge a branch into the main branch. In this case however, you merge a branch into multiple destination branches.

@krivard
Copy link
Contributor Author

krivard commented Aug 26, 2020

We're going to handle this by establishing the following workflow:

  • Branch from main to create a feature branch
  • PR against deployment branch for code reviews and test checks
  • PR against main

@krivard krivard closed this as completed Aug 26, 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

3 participants