-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
ci: make git-diff-develop work for PRs from foreign repos #27268
Conversation
CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes. |
b316eca
to
3e6d1ba
Compare
74bfede
to
c38a9c4
Compare
@@ -61,7 +56,7 @@ async function fetchUntilMergeBaseFound() { | |||
} catch (error: unknown) { | |||
if ( | |||
error instanceof Error && | |||
hasProperty(error, 'code') && | |||
Object.hasOwnProperty.call(error, 'code') && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hasProperty
is the only function being required from an external module. By inlining it, we can remove the dependency on prep-deps
in CI.
e051da3
to
167babe
Compare
167babe
to
c8a56aa
Compare
c8a56aa
to
256a561
Compare
Did you test this with a changed |
Will this do? Also covering the locale-file checking (usually only running on crowdin prs), which also depends on the job. |
3827bf4
to
58e0bf9
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is going to break non-forks, see this: https://support.circleci.com/hc/en-us/articles/360047521451-Why-is-CIRCLE-PR-NUMBER-empty
CIRCLE_PR_NUMBER is only defined in forks, yeah it's super weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Once the PR_NUMBER issue is resolved, I can approve. I like how Howard solved that issue in #27548
This partially fixes #27135 @itsyoboieltr is working on getting SonarCloud to run on forks, which is the other task in that issue. |
The gh API requires authentication, even if the API endpoints do not. We can just fetch the PR URL directly without shelling out.
58e0bf9
to
c554b95
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@@ -82,9 +95,11 @@ async function fetchUntilMergeBaseFound() { | |||
*/ | |||
async function gitDiff(): Promise<string> { | |||
await fetchUntilMergeBaseFound(); | |||
const { stdout: diffResult } = await exec(`git diff --name-only origin/HEAD...${process.env.CIRCLE_BRANCH}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm noticing a problem here: we're comparing against the base branch, not the base commit. And similarly, we're comparing the current branch, not the current commit. This is problematic because the results are now dependent upon the exact timing of when this runs, and they're comparing changes made to develop that aren't in the target branch yet.
We're using this diff as a measure of what has changed in the PR, but because we're not comparing with the correct commit, we're going to get skewed results (as changes are made to develop
, or as develop
is merged into the PR, etc.). This problem is in the original implementation as well though, not introduced here.
I will follow up on this separately. We have fixed this same problem already elsewhere, we can fix it here too without much effort.
Edit: On second thought, it's just the reference to the current branch that is problematic, because when a later commit is made it would impact the CI run of prior commits.
Using origin/develop
as the point of comparison is less problematic. In theory it still makes this less deterministic, but maybe not in a way that matters (e.g. it could result in the diff getting smaller if some portion of the branch was merged as a separate PR, but the results would be equally "correct" in a sense).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this is just used to run modified tests additional times, I'm having a hard time imagining a scenario where this has a negative impact, despite it being technically incorrect. I'm thinking maybe this is fine, not worth fixing for now.
I'm force-merging this to get around the failing Sonar Cloud check, which is broken for forks. @legobeat I would have liked to wait until you had a chance to review that last commit, but it's fairly similar to Howard's other PR (which you had approved) so I'm assuming you're good with this approach. My apologies if that assumption is incorrect. |
@Gudahtt & @HowardBraham : All good, thanks to both of you for straightening this out and getting it in! |
No release label on PR. Adding release label release-12.6.0 on PR, as PR was added to branch 12.6.0 when release was cut. |
The
gh
CLI requires authentication, even if the API endpoints do not. We can just fetch the PR URL directly without shelling out.Fixes CI error.
Also removes dependency on
prep-deps
step by removing dependency on external module.