-
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: followup to CircleCI Sentry reporting #27548
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. |
0839055
to
a6d14e0
Compare
Builds ready [a6d14e0]
Page Load Metrics (1780 ± 96 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
f8bbc5b
to
7f0edcf
Compare
} | ||
return diffResult; | ||
} | ||
|
||
function writePrBodyToFile(prBody: string) { |
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.
Q: Why is the PR body written to file? Is there something that prevents it from being passed differently?
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.
Good question. This needs to get to all of the test-e2e-*
VMs. I'm trying to avoid calling api.github.com hundreds of times, and instead store it in artifacts
.
Builds ready [18502f9]
Page Load Metrics (2089 ± 186 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
Builds ready [038a11c]
Page Load Metrics (1930 ± 97 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
What is the intended purpose of this feature? i.e. when would we want to update the Sentry trace rate from a PR description. I see that this capability was added already in #27412, but I don't see a reference to its purpose there either. Edit: I see, tracing is entirely disabled on PRs by default, so this would be for getting performance metrics from PRs I suppose. Makes 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.
Generally this looks good! But it would be ideal to make the unrelated changes to git-diff-develop
in a separate PR. This is recommended by our contributor guidelines not just to make review easier and more effective, but also to make the git history easier to audit.
038a11c
to
2d953bf
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!
Quality Gate failedFailed conditions |
Builds ready [2d953bf]
Page Load Metrics (1860 ± 66 ms)
Bundle size diffs [🚀 Bundle size reduced!]
|
@@ -238,7 +238,7 @@ function getSentryEnvironment() { | |||
|
|||
function getSentryTarget() { | |||
if ( | |||
getManifestFlags().sentry?.doNotForceSentryForThisTest || | |||
!getManifestFlags().sentry?.forceEnable || |
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.
We need to change the ||
here to &&
because we flipped the meaning of the flag
Description
Implementing some suggestions from @matthewwalsh0 from #27412
Also incorporates changes from @legobeat's #27268doNotForceSentryForThisTest
todoNotForceForThisTest
because theSentry
is now implied by the parent propertyaddFlagsFromPrBody()
andaddFlagsFromGitMessage()
functionstracesSampleRate
) but it's built to be easily extendable for anythingflags = {"sentry": {"tracesSampleRate": x.xx}}
If you do both, Git commit message takes precedence
[flags.sentry.tracesSampleRate: x.xx]
toflags = {"sentry": {"tracesSampleRate": x.xx}}
Note: This PR, as is, will hit the following error because it's trying to actually parse the sample code above with
x.xx
. The good news is it fails gracefully.Related issues
Followup to: #27412
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist