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

chore: clearly state system.url-prefix value should be set #3100

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

aldy505
Copy link
Collaborator

@aldy505 aldy505 commented Jun 1, 2024

There are more than one cases of people having their DSN or email links empty, most of it because system.url-prefix is not set on the config. Hope this will solves that (by providing a clearer message). But, I guess we should add this one to the docs site too, regarding "which config file you should be modifying before you run your sentry".

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

Copy link

codecov bot commented Jun 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.01%. Comparing base (b8b4aa2) to head (cd2aef7).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #3100   +/-   ##
=======================================
  Coverage   99.01%   99.01%           
=======================================
  Files           3        3           
  Lines         203      203           
=======================================
  Hits          201      201           
  Misses          2        2           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BYK
Copy link
Member

BYK commented Jun 1, 2024

This should be handled by the initial set up screen, that's why we didn't ask it to be set in the config.

Agree with docs update but disagree with this specific change. If there's an issue with the set-up flow we should address that instead (on get sentry/sentry side)

@aldy505
Copy link
Collaborator Author

aldy505 commented Jun 4, 2024

This should be handled by the initial set up screen, that's why we didn't ask it to be set in the config.

Agree with docs update but disagree with this specific change. If there's an issue with the set-up flow we should address that instead (on get sentry/sentry side)

I'm quite pessimistic on having that changed on the sentry/sentry side, since this is a common issue that most people encountered during their first time self-hosting sentry.

@BYK
Copy link
Member

BYK commented Jun 4, 2024

I'm mostly trying to understand how this is not being set in the initial setup. Do we have any links to people asking about this so I can check?

@aldy505
Copy link
Collaborator Author

aldy505 commented Jun 4, 2024

Do we have any links to people asking about this so I can check?

A few that I remembered:

@BYK
Copy link
Member

BYK commented Jun 14, 2024

Sorry for the late turnaround here. How about we try to do //github.com//issues/2621#issuecomment-1842014431 and the subsequent comment instead? Hard-coding makes changing this a bit more tricky (although I agree that it should not be something that is changed regularly)

@aldy505
Copy link
Collaborator Author

aldy505 commented Jun 24, 2024

Sorry for the late turnaround here. How about we try to do //github.com//issues/2621#issuecomment-1842014431 and the subsequent comment instead? Hard-coding makes changing this a bit more tricky (although I agree that it should not be something that is changed regularly)

I don't really get what you're trying to do here.. since I believe modifying through the web app is something that's not persisted on disk (to postgres or file). But again, I'm not so sure about that, hence I prefer it to be done via configuration file.

Don't get me wrong: I wanted the modified configurations to survive between updates, but I don't know whether it will correctly survive that.

@BYK
Copy link
Member

BYK commented Jun 25, 2024

since I believe modifying through the web app is something that's not persisted on disk (to postgres or file).

It should persist on Postgres, or somewhere persistent as that's how the options system works. I'll list the relevant code for reference:

  1. https://github.com/getsentry/sentry/blob/accca8f98c4231dae9a484c8985da43e7a6c4645/src/sentry/options/defaults.py#L63
  2. https://github.com/getsentry/sentry/blob/accca8f98c4231dae9a484c8985da43e7a6c4645/src/sentry/options/manager.py#L304
  3. https://github.com/getsentry/sentry/blob/accca8f98c4231dae9a484c8985da43e7a6c4645/static/app/views/admin/adminSettings.spec.tsx#L15

Now in number 1 and 3, you can see that it will be disabled on the UI when it was set from the disk (options file). That said I cought something quite interesting: looks like we fetch the default value from SENTRY_SYSTEM_URL_PREFIX so I'd now encourage setting that (in .env maybe) and documenting it.

WDYT?

@aldy505
Copy link
Collaborator Author

aldy505 commented Jun 30, 2024

since I believe modifying through the web app is something that's not persisted on disk (to postgres or file).

It should persist on Postgres, or somewhere persistent as that's how the options system works. I'll list the relevant code for reference:

  1. https://github.com/getsentry/sentry/blob/accca8f98c4231dae9a484c8985da43e7a6c4645/src/sentry/options/defaults.py#L63
  2. https://github.com/getsentry/sentry/blob/accca8f98c4231dae9a484c8985da43e7a6c4645/src/sentry/options/manager.py#L304
  3. https://github.com/getsentry/sentry/blob/accca8f98c4231dae9a484c8985da43e7a6c4645/static/app/views/admin/adminSettings.spec.tsx#L15

Now in number 1 and 3, you can see that it will be disabled on the UI when it was set from the disk (options file). That said I cought something quite interesting: looks like we fetch the default value from SENTRY_SYSTEM_URL_PREFIX so I'd now encourage setting that (in .env maybe) and documenting it.

WDYT?

Putting on the .env file might work. Seeing that, do you still prefer setting it on the .env file for visibility, or we'll just leave the additional comment for the configuration file?

@hubertdeng123 @azaslavsky what's your thoughts about this?

@BYK
Copy link
Member

BYK commented Jul 1, 2024

Putting on the .env file might work. Seeing that, do you still prefer setting it on the .env file for visibility, or we'll just leave the additional comment for the configuration file?

I feel like .env is better because:

  1. It is more visible as you suggest
  2. It does not count as "on disk" so should allow changing from the UI

@aldy505 aldy505 marked this pull request as draft July 9, 2024 00:52
@aldy505 aldy505 marked this pull request as ready for review September 8, 2024 00:15
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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants