doc-base
doc-base copied to clipboard
php
Merge to master before running tests
When a PR is branched from a point before current master, the CI tests are run ignoring any intermediary commits. In other words, tests are running considering one version that will never exist in practice.
To avoid this, run an local git merge from the branched checkout. This is a no-op if there is no commits to be merged.
Well, isn't that expected behavior?
I would argue is not expected behavior that CI tests test a version of code that will never exist.
https://github.com/php/doc-base/pull/205#issuecomment-2541033072
Taking a branch from before the tip of master is no different of taking five branchs of same tip of master, and then merging then in any order. The first merge will not show this problem, but for the other fours are now effectively branched not from from the tip of master, but from a previous version. And CI will ignore any of this, will test the code excluding any changes to master, and then, when merged, the code merged is not the code tested.
Is there a particular reason to work-around that only for doc-base?
The doc-base is checked out from a branch, all other repos are already checked from master.
The doc-base is checked out from a branch, all other repos are already checked from master.
I was not particularly referring to doc-*, but rather to "all" GH repos using the checkout action (not only those of the php organization). It seems to me that it is relatively rare that issues arise due to not testing against the HEAD (+ the PR commits), and this can be resolved with a rebase.
That said, I'm not against your suggested change.
I was not particularly referring to doc-*, but rather to "all" GH repos using the checkout action (not only those of the php organization).
That is something I asked on https://github.com/php/doc-base/pull/205#issuecomment-2541082165 , and got no answers.
It seems to me that it is relatively rare that issues arise due to not testing against the HEAD (+ the PR commits), and this can be resolved with a rebase.
It's rare, yet it happened, in doc-base no less.
Any CI that uses actions/checkout@v4 assumes that there are no concurrent PRs, or, in alternative, that all CI tests only make sense if executed from a rebased state. Yet, GitHub CI tests are not blocked or discarded on non-rebased PRs. It may be worth raising an issue in GitHub Actions Marketplace, asking for an option to something like merge: master, so projects with many concurrent PRs may have a better experience with GH CI.
Checking out the checkout action PR and issues, there various discussions for and against merge commit checkout, with the implication that is the merge is the default options. But experience with PR 205 showed it was note the case. After this PR is merged, I will try recreate what happened in PR 205.
I think I confirmed it. Besides the checkout being made from refs/remotes/pull/217/merge, that implies that some merging being done, and docs mentioning merge commits, after others PRs are merged, this PR started failing CI.
Not because of a merge conflict, but because of an Committer identity unknown error in git -C doc-base merge --no-ff --no-commit origin/master. In other words, the merge found differences between the CI checkout and master, and is trying to create a merge commit for it (even in the presence of --no-commit ...). So if some merging is being done, it ignores master history in some (all?) cases.
Next I will try some fixes for Committer identity unknown that does not involve persisted configurations, to see what the merge command is detecting.
Sorry for the noise. I think this may be a hard problem to solve, as actions/checkout@v4 uses a refspec that generates a detached HEAD state, which makes it harder to coordinate further fetch/checkout/merges of master.
Also, the problem is really about a cached state. Even after changes are made in master, the CI still generates the same hash for the merge commit, indicating that it may be executing tests in a behind version of code. At the same time, any changes to PR flushes/renews the cached result, so very act of developing a fix "fixes" the problem, at least temporarily. I will try to simulate the problem and the fix in a particular repository.
Meanwhile, I changed the PR to a test that will intentionally fail, if the merge commit targets a hash that is not the head of master.
An old teste generated:
HEAD is now at 478a8242 Merge c496e97c7b89e491b6b5d2179441ff9dba4cf13a into 07b48068fa77eebcb6c1e2788c9a8026fa1bfcae
All tests pass.
Running test same test after merge 218:
HEAD is now at 478a8242 Merge c496e97c7b89e491b6b5d2179441ff9dba4cf13a into 07b48068fa77eebcb6c1e2788c9a8026fa1bfcae
All tests fail.
After a commit make insignificant chantes on PR:
HEAD is now at 53c56510 Merge b7b547b0877092f81a17eb7257cc634fa7ec49b5 into 0ae92dd1987e98a62c7d464bb0659a9e63ffe794
All tests pass.
So, only to repeat. GitHub actions/checkout@v4 ignores changes on main/master for calculating the commit merge target.
And GitHub actions/checkout@v4 recalculate the commit merge target on PR code changes.
This PR, as it is now, only refuses to run CI when the commit merge target is outdated.
Yet, GitHub CI tests are not blocked or discarded on non-rebased PRs.
This is configurable per repository. Attempting to solve this in the GitHub Action configuration is the wrong place, because the result will get stale between running the CI and actually merging the PR.
Yet, GitHub CI tests are not blocked or discarded on non-rebased PRs.
This is configurable per repository.
That's good to hear. What is the name of a configuration that blocks and discards PRs testing that targets behind/old/stale master. Because...
Attempting to solve this in the GitHub Action configuration is the wrong place, because the result will get stale between running the CI and actually merging the PR.
This is already happening, as of now. The CI testing is targeting behind/old/stale master. Merging changes of one PR does not change the results of other open PR tests, even when re-running the tests.
That's good to hear. What is the name of a configuration that blocks and discards PRs testing that targets behind/old/stale master. Because...
This is part of the branch rulesets, more specifically the “Green CI” requirement (because without CI it doesn't make sense to force the branch being up to date):
There is also another setting that just suggests updating the branch if it is outdated:
This is already happening, as of now. The CI testing is targeting behind/old/stale master. Merging changes of one PR does not change the results of other open PR tests, even when re-running the tests.
Yes. But this PR does not improve on the situation in a meaningful way.
This is part of the branch rulesets, more specifically the “Green CI” requirement (because without CI it doesn't make sense to force the branch being up to date):
And for that not to be a nuisance you need to use the “Merge Queue”, because otherwise you are constantly updating all PRs, even if there it not the slightest chance of conflict:
There is also another setting that just suggests updating the branch if it is outdated:
This appears to be a nice option. But do you know if it:
- updates only the PR; or
- also update GH fork?
In both cases, I can see this option causing conflicts with the local checkout.
This is part of the branch rulesets, more specifically the “Green CI” requirement (because without CI it doesn't make sense to force the branch being up to date):
From the documentation:
I interpreted from these documents that this option only blocks merging of outdated/not rebased forks, while allowing running CIs, even with incompatible changes.
Yes. But this PR does not improve on the situation in a meaningful way.
I think this PR address a slightly different problem. It focuses on CI always having the same results, be it running on rebased or non-rebased PRs.
This appears to be a nice option. But do you know if it:
* updates only the PR; or * also update GH fork?
I'm not sure if I understand the question. The branch contents are the authoritative source of the PR. Pressing the button would perform a merge of the target branch into the source branch, just as if you do this locally with git.
I interpreted from these documents that this option only blocks merging of outdated/not rebased forks, while allowing running CIs, even with incompatible changes.
The CI only runs the latest commit in a given PR. But by requiring the option, GitHub ensures that prior to merge, the PR is a strict superset of the target branch, thus ensuring that all changes in the target branch are accounted for within the PR. So yes, the CI might run on outdated information, but if it ran on outdated information, merging is blocked. So you get the desired result.
I think this PR address a slightly different problem. It focuses on CI always having the same results, be it running on rebased or non-rebased PRs.
I think this PR does not address any problem at all and just adds additional problems / confusion, because the CI will behave unexpectedly, by not testing the latest commit of a PR as it would do for every other PR using GitHub Actions.
I was hoping to block new CI runs on outdated branches, and possibly invalidate old CI runs of outdated branchs.
Closing without merge, as https://github.com/php/doc-base/pull/226 is about to supersede this anyways.
The only note is that the fetch-depth: 0 / git merge --no-ff --no-commit origin/master trick can avoid generating or regenerating CI results that will never exist in master/main.