Skip to content

ci: Fix size check action to run on master #6248

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

Merged
merged 1 commit into from
Nov 21, 2022
Merged

Conversation

mydea
Copy link
Member

@mydea mydea commented Nov 21, 2022

As pointed out by @AbhiPrasad, the size check action on CI has not been properly running since we merged #6180.

After some investigation, I think I've pinned the culprit down:

Basically, in that PR I "fixed" an if condition for the size limit action job. Previously, because the condition had a wrong syntax, it was always running.

if: ${{ github.event_name == 'pull_request' }} || ${{ startsWith(github.ref, 'refs/heads/master/') }}

This was incorrect because either we need to wrap the whole expression with ${{ }}, or none of it (as if auto-wraps it). With the original statement, it always evaluated to a truthy expression.

So I "fixed" the syntax, however didn't notice that one part of the check was incorrect:

if: github.event_name == 'pull_request' || startsWith(github.ref, 'refs/heads/master/')

Actually, refs/heads/master/ never matches - the trailing / is incorrect, I believe - should be:

if: github.event_name == 'pull_request' || github.ref == 'refs/heads/master'

So this PR actually fixes that statement with the proper branch name (hopefully). Also, as far as I can tell from github docs, we don't actually need the startsWith stuff here.

@mydea mydea added the Dev: CI label Nov 21, 2022
@mydea mydea requested a review from a team November 21, 2022 13:39
@mydea mydea self-assigned this Nov 21, 2022
@mydea mydea requested review from Lms24 and lobsterkatie and removed request for a team November 21, 2022 13:39
Copy link
Member

@AbhiPrasad AbhiPrasad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking a look!

@mydea mydea enabled auto-merge (squash) November 21, 2022 13:43
@github-actions
Copy link
Contributor

size-limit report 📦

Path Size
@sentry/browser - ES5 CDN Bundle (gzipped + minified) 19.52 KB (added)
@sentry/browser - ES5 CDN Bundle (minified) 60.37 KB (added)
@sentry/browser - ES6 CDN Bundle (gzipped + minified) 18.17 KB (added)
@sentry/browser - ES6 CDN Bundle (minified) 53.72 KB (added)
@sentry/browser - Webpack (gzipped + minified) 19.91 KB (added)
@sentry/browser - Webpack (minified) 65.18 KB (added)
@sentry/react - Webpack (gzipped + minified) 19.94 KB (added)
@sentry/nextjs Client - Webpack (gzipped + minified) 45.93 KB (added)
@sentry/browser + @sentry/tracing - ES5 CDN Bundle (gzipped + minified) 26.36 KB (added)
@sentry/browser + @sentry/tracing - ES6 CDN Bundle (gzipped + minified) 24.77 KB (added)

@mydea mydea merged commit 955a1b2 into master Nov 21, 2022
@mydea mydea deleted the fn/fix-size-action branch November 21, 2022 13:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants