ci: ignore bundle size reporter failures #32490
Conversation
facebook-github-bot
added
CLA Signed
|
@sota000 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
| - run: | ||
| name: Report size of RNTester.app (analysis-bot) | ||
| command: GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" scripts/circleci/report-bundle-size.sh << parameters.platform >> | ||
| command: GITHUB_TOKEN="$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_A""$PUBLIC_ANALYSISBOT_GITHUB_TOKEN_B" scripts/circleci/report-bundle-size.sh << parameters.platform >> || true |
There was a problem hiding this comment.
Where did CircleCI mention it doesn't populate CIRCLE_PULL_REQUEST anymore? I still see it in their docs -- as well, the tests look to still be passing for pull requests. ex: https://app.circleci.com/pipelines/github/facebook/react-native/10904/workflows/8ef9199f-0dfd-4a09-a74c-73704d98a7ae/jobs/223365
We do know that CIRCLE_PULL_REQUEST won't be set on branches that aren't pull requests -- like the release branches -- are you seeing something different?
There was a problem hiding this comment.
This is from the build logs of this PR: https://app.circleci.com/pipelines/github/facebook/react-native/10897/workflows/8c3727af-40be-48be-8c97-30b77f9a0608/jobs/223290/parallel-runs/0/steps/0-125
The size reporter failed. If we look at the environment variables, CIRCLE_PULL_REQUEST isn't set: https://app.circleci.com/pipelines/github/facebook/react-native/10897/workflows/8c3727af-40be-48be-8c97-30b77f9a0608/jobs/223290/parallel-runs/0/steps/0-99
To be clear, it still gets populated if your PR comes from a fork. Here's the list of env variables from the build you posted: https://app.circleci.com/pipelines/github/facebook/react-native/10904/workflows/8ef9199f-0dfd-4a09-a74c-73704d98a7ae/jobs/223364/parallel-runs/0/steps/0-99
CIRCLE_PULL_REQUEST isn't set when the PR comes from a branch in this repo. I suspect CircleCI changed it to match what they do with CIRCLE_PR_NUMBER (only available on forked PRs), but didn't update the docs. Or they accidentally changed it and haven't realized it yet. Regardless, size reporting shouldn't fail builds. I first noticed it in #32489.
There was a problem hiding this comment.
Ohoo I see.
Is it possible for us to check the existence of CIRCLE_PULL_REQUEST in report-bundle-size.sh and log out that we're skipping if it's unset and no-op there? I think it'd be clearer and we'd skip some work
There was a problem hiding this comment.
That's what it's doing already though. It is telling us that it's missing the PR number and exits early. It doesn't do any unnecessary work. The problem is that we're treating it as an error and fail the whole pipeline. I don't think it's necessary to block a PR because such errors are not actionable. It could have been a network issue, or Firebase could be down as well.
There was a problem hiding this comment.
So this error:
Missing GITHUB_PR_NUMBER. Example: 4687. PR feedback cannot be provided on GitHub without a valid pull request number.
is coming from here:
react-native/bots/make-comment.js
Line 79 in 1465c8f
Which is called from here:
react-native/bots/report-bundle-size.js
Line 112 in 1465c8f
which happens after we've read all the file sizes which we could maybe skip. We can add a PR number check in report-bundle-size.js as well? What do you think?
There was a problem hiding this comment.
Ah, you're right, of course! I assumed that it was checked, but we only check GITHUB_REF and GITHUB_SHA. Good call.
There was a problem hiding this comment.
Aaand CIRCLE_PULL_REQUEST is back. Guess it was a mistake after all 😄
There was a problem hiding this comment.
Oh whoops, didn't see this comment --I think this is still a worthwhile change, I can extract the validation and update the post-artifacts-link to get this off your plate
There was a problem hiding this comment.
Thanks, but I don't think it would be right to break post-artifacts-link. I didn't know make-comment was used elsewhere. It should be fixed now.
Base commit: b25d9e4 |
Base commit: b25d9e4 |
|
@lunaleaps has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator. |
CircleCI stopped populating `CIRCLE_PULL_REQUEST` without providing an alternative, so now it's impossible to get the PR number unless it comes from a forked repo.
tido64
force-pushed
the
tido/fix-bundle-size-reporter
branch
from
f63a1ac to
33695c2
Compare
|
@lunaleaps merged this pull request in 8649174. |
5 participants
Summary
CircleCI stopped populating
CIRCLE_PULL_REQUESTwithout providing analternative, so now it's impossible to get the PR number unless it comes
from a forked repo.
Changelog
[Internal] [Fixed] - ignore bundle size reporter failures
Test Plan
CI should ignore bundle size reporter failures.