Skip to content
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

Skip saucelabs for community contributions #2321

Merged
merged 13 commits into from
Oct 25, 2022

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Oct 24, 2022

#skip-changelog

E.g. this PR has failed because there are no secrets available in the contributor's repo. #2299

@vaind do you know if there's a better way to do it?

@philipphofmann
Copy link
Member

Not running SauceLabs for contributors at all is not great cause SauceLabs is there to find bugs. I'm not sure right now how to fix this either.

@codecov-commenter
Copy link

codecov-commenter commented Oct 24, 2022

Codecov Report

Base: 80.21% // Head: 80.21% // No change to project coverage 👍

Coverage data is based on head (6f5879c) compared to base (f6029be).
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #2321   +/-   ##
=========================================
  Coverage     80.21%   80.21%           
  Complexity     3475     3475           
=========================================
  Files           247      247           
  Lines         12906    12906           
  Branches       1735     1735           
=========================================
  Hits          10352    10352           
  Misses         1894     1894           
  Partials        660      660           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@romtsn
Copy link
Member Author

romtsn commented Oct 24, 2022

there's no fix https://docs.github.com/en/actions/security-guides/encrypted-secrets#using-encrypted-secrets-in-a-workflow. Apart from pulling the branch in and reopening the PR ourselves, but this also sucks.

@vaind
Copy link
Collaborator

vaind commented Oct 24, 2022

Running CI with secrets involved on forks is indeed problematic. As described in this article, there should be an option to use workflow_run trigger which would allow us to use secrets within defined within the main repo with artefacts built by a workflow running on a fork. However, this seems a bit more complicated so not sure it's worth it.

FWIW in dart I've changed the CI to run on push instead of pull_request: https://github.com/getsentry/sentry-dart/pull/1075/files

@github-actions
Copy link
Contributor

github-actions bot commented Oct 24, 2022

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 292.00 ms 349.71 ms 57.71 ms
Size 1.73 MiB 2.32 MiB 608.62 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
4dd88fe 306.88 ms 391.58 ms 84.70 ms
649f171 300.58 ms 367.44 ms 66.86 ms
c5ccd8a 329.98 ms 365.52 ms 35.54 ms
f6029be 246.18 ms 340.29 ms 94.10 ms
2c5f172 310.20 ms 357.16 ms 46.96 ms
7d87f22 348.79 ms 378.46 ms 29.67 ms
3d89dea 322.38 ms 350.82 ms 28.45 ms
1e4690d 354.69 ms 387.88 ms 33.19 ms
7300956 337.57 ms 384.21 ms 46.64 ms
d4087ee 278.00 ms 313.86 ms 35.86 ms

App size

Revision Plain With Sentry Diff
4dd88fe 1.73 MiB 2.29 MiB 579.50 KiB
649f171 1.73 MiB 2.32 MiB 608.44 KiB
c5ccd8a 1.74 MiB 2.33 MiB 607.44 KiB
f6029be 1.73 MiB 2.32 MiB 608.62 KiB
2c5f172 1.73 MiB 2.29 MiB 580.10 KiB
7d87f22 1.73 MiB 2.29 MiB 580.01 KiB
3d89dea 1.74 MiB 2.33 MiB 604.92 KiB
1e4690d 1.74 MiB 2.33 MiB 604.92 KiB
7300956 1.73 MiB 2.29 MiB 578.69 KiB
d4087ee 1.73 MiB 2.29 MiB 579.50 KiB

Previous results on branch: chore/skip-saucelabs-for-contributors

Startup times

Revision Plain With Sentry Diff
316b38b 322.31 ms 352.88 ms 30.57 ms
56aa6fe 295.88 ms 343.37 ms 47.50 ms

App size

Revision Plain With Sentry Diff
316b38b 1.73 MiB 2.32 MiB 608.62 KiB
56aa6fe 1.73 MiB 2.32 MiB 608.64 KiB

@philipphofmann
Copy link
Member

FWIW in dart I've changed the CI to run on push instead of pull_request: https://github.com/getsentry/sentry-dart/pull/1075/files

@vaind, what's the benefit of doing that?

@vaind
Copy link
Collaborator

vaind commented Oct 25, 2022

@vaind, what's the benefit of doing that?

not much in terms of benefits - it's just another way of doing the same, i.e. not running when there's no secret. Maybe a slight benefit would be it would run (and fail) on this repo branches where the secret is supposed to be defined, as opposed of the approach in this PR where if someone were to delete the secret, the workflow would just silently skip running tests.

@romtsn romtsn merged commit 0fbe535 into main Oct 25, 2022
@romtsn romtsn deleted the chore/skip-saucelabs-for-contributors branch October 25, 2022 12:07
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

Successfully merging this pull request may close these issues.

5 participants