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.

Sign up for GitHub

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

Jump to bottom

Use SENTRY_SPOTLIGHT as default value for Spotlight options #1789

Merged
merged 8 commits into from
Dec 10, 2024
Merged

Use SENTRY_SPOTLIGHT as default value for Spotlight options #1789

merged 8 commits into from
Dec 10, 2024

Conversation

stayallive
Copy link
Collaborator

@stayallive stayallive commented Nov 14, 2024 •
edited by BYK
Loading

This reads SENTRY_SPOTLIGHT environment variable as source of truth and also allows passing the URL directly to the spotlight option.

Related getsentry/spotlight#545 and getsentry/spotlight#482

@stayallive stayallive force-pushed the spotlight-url-from-env branch from 6b9a5fd to 235f9b7 Compare November 14, 2024 11:49
@stayallive stayallive force-pushed the spotlight-url-from-env branch from 235f9b7 to fd5d15d Compare November 14, 2024 11:50
@stayallive stayallive marked this pull request as ready for review November 14, 2024 11:51
@stayallive stayallive requested a review from cleptric November 14, 2024 11:51
BYK
BYK reviewed Nov 14, 2024
View reviewed changes
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

LGTM but won't approve until someone from the PHP SDK team takes a look at this.

src/Options.php Show resolved Hide resolved
tests/OptionsTest.php Show resolved Hide resolved
@BYK
Copy link
Member

BYK commented Nov 19, 2024

@cleptric ?

@cleptric
Copy link
Member

@BYK ?

@BYK
Copy link
Member

BYK commented Nov 19, 2024

Lol -- @cleptric was wondering if you have any objections as last time we talked about this you seemed on the fence for making $SENTRY_SPOTLIGHT being both boolean and a string. I can approve and get this merged but wanted to get your blessing first.

@cleptric
Copy link
Member

My opinion on this didn't change, but I won't object these changes.
Once @stayallive added the deprecation annotations, we can get this merged.

BYK
BYK previously requested changes Nov 19, 2024
View reviewed changes
Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Sending back for deprecation changes.

cleptric
cleptric reviewed Nov 19, 2024
View reviewed changes
src/Options.php Outdated Show resolved Hide resolved
@stayallive stayallive requested a review from BYK November 28, 2024 12:20
cleptric
cleptric approved these changes Nov 28, 2024
View reviewed changes
src/Options.php Outdated Show resolved Hide resolved
src/Options.php Outdated Show resolved Hide resolved
stayallive and others added 2 commits November 30, 2024 16:55
@stayallive @cleptric
Update src/Options.php
2a5022a 
Co-authored-by: Michi Hoffmann <cleptric@users.noreply.github.com>
@stayallive @cleptric
Update src/Options.php
1580c95 
Co-authored-by: Michi Hoffmann <cleptric@users.noreply.github.com>
@cleptric cleptric dismissed BYK’s stale review December 10, 2024 12:47

Requested changes were made.

@cleptric cleptric enabled auto-merge (squash) December 10, 2024 12:47
@cleptric cleptric merged commit 9b773f5 into master Dec 10, 2024
37 checks passed
@cleptric cleptric deleted the spotlight-url-from-env branch December 10, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jean85 Jean85 Jean85 left review comments

@cleptric cleptric cleptric approved these changes

@BYK BYK Awaiting requested review from BYK

Assignees
No one assigned
Labels
Type: Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@stayallive @BYK @cleptric @Jean85