Rename onpremise to self-hosted in configs #30528

chadwhitacre · 2021-12-09T12:21:38Z

Big things to think about here are backcompat for both getsentry and self-hosted users. Customizations? Plugins? FE? BE?

github-actions · 2021-12-09T12:23:55Z

size-limit report

Path	Base Size (81cc160)	Current Size	Change
src/sentry/static/sentry/dist/entrypoints/app.js	52.88 KB	52.88 KB	+0.02% 🔺
src/sentry/static/sentry/dist/entrypoints/sentry.css	70.88 KB	70.88 KB	0%

chadwhitacre · 2021-12-09T15:16:08Z

k so I'm gonna ignore those codecov reds. 👀

The visual diff though seems actually relevant, let me make sure I understand what this is doing ...

chadwhitacre · 2021-12-09T15:48:29Z

src/sentry/web/helpers.py

@@ -20,7 +20,8 @@ def get_default_context(request, existing_context=None, team=None):
        "URL_PREFIX": options.get("system.url-prefix"),
        "SINGLE_ORGANIZATION": settings.SENTRY_SINGLE_ORGANIZATION,
        "PLUGINS": plugins,
-        "ONPREMISE": settings.SENTRY_ONPREMISE,
+        "ONPREMISE": settings.SENTRY_SELF_HOSTED,  # deprecated, use ...
+        "SELF_HOSTED": settings.SENTRY_SELF_HOSTED,  # ... this instead


What are these contexts? What consumes them?

Introduced in #3350 so Django templates can vary:

sentry/src/sentry/templates/sentry/layout.html

Line 193 in e99df7f

{% if ONPREMISE %}<a href="/out/">{% trans "Migrate to SaaS" %}</a>{% endif %}

We're not using this anymore, but if we remove this then we should probably remove SINGLE_ORGANIZATION as well, but I want to be conservative here so I am leaving these in.

chadwhitacre · 2021-12-09T15:54:11Z

src/sentry/conf/server.py

+# for now, but in the rest of _this_ codebase we should reference
+# SENTRY_SELF_HOSTED.
+SENTRY_ONPREMISE = True  # deprecated, use ...
+SENTRY_SELF_HOSTED = SENTRY_ONPREMISE  # ... this instead


This is the risky part. We need to make sure that getsentry (and I guess whatever goofy third-party stuff exists out in the wild) doesn't break. As we can see, SENTRY_ONPREMISE defaults to true, and probably nothing but getsentry overrides that.

Not too important for this pr, but if we want to remove SENTRY_ONPREMISE we should also check with the single tenant team to see whether they set this variable differently as well.

Good call @wedamija. This came up at #30528 (comment) as well, and I've put single-tenant on the pile at getsentry/self-hosted#796 to make sure it isn't forgotten. Close one!

chadwhitacre · 2021-12-09T15:58:22Z

static/app/views/settings/settingsIndex.tsx

      href: LINKS.FORUM,
      to: `${organizationSettingsUrl}support`,
    };
-    const supportText = isOnPremise ? t('Community Forums') : t('Contact Support');
+    const supportText = isSelfHosted ? t('Community Forums') : t('Contact Support');


Actually I should update the forums link as well to keep up with getsentry/self-hosted#1151, but that's out of scope for this repo.

chadwhitacre · 2021-12-09T15:59:57Z

static/app/plugins/sessionstack/components/settings.tsx

@@ -7,12 +7,12 @@ import DefaultSettings from 'sentry/plugins/components/settings';
 type Props = DefaultSettings['props'];

 type State = DefaultSettings['state'] & {
-  showOnPremisesConfiguration?: boolean;
+  showSelfHostedConfiguration?: boolean;


This ancient plugin has a few config values that are only relevant on self-hosted. This is the source of the screenshots that changed.

chadwhitacre · 2021-12-09T16:00:27Z

src/sentry_plugins/sessionstack/plugin.py

@@ -123,7 +123,7 @@ def get_config(self, project, **kwargs):
            },
        ]

-        if settings.SENTRY_ONPREMISE:
+        if settings.SENTRY_SELF_HOSTED:


Ancient plugin. See settings.tsx.

chadwhitacre · 2021-12-09T16:02:49Z

src/sentry/web/client_config.py

@@ -153,7 +153,8 @@ def get_client_config(request=None):
        "statuspage": _get_statuspage(),
        "messages": [{"message": msg.message, "level": msg.tags} for msg in messages],
        "apmSampling": float(settings.SENTRY_FRONTEND_APM_SAMPLING or 0),
-        "isOnPremise": settings.SENTRY_ONPREMISE,
+        "isOnPremise": settings.SENTRY_SELF_HOSTED,  # deprecated, use ...
+        "isSelfHosted": settings.SENTRY_SELF_HOSTED,  # ... this instead


Second-riskiest part. getsentry does consume the isOnPremise config key in some places. Third-party JavaScript (plugins?) might as well. Preserve the key!

Will we be removing isOnPremise after getsentry is updated?

I'd like to, but I'm not sure how much impact that has on self-hosters. Probably not much? I'd like to at least get one release out with both keys and a notice in the changelog, then remove "sometime" after that. Thoughts?

I'd imagine it shouldn't affect them, if they are customizing their install, they shouldn't have a need to differentiate between the two? But otherwise sounds good

I mean the default is true. Nobody except us in SaaS should need to change the default. Also yes, like you say, nobody in the wild building their own plugins or whatever should need to vary their behavior on whether this config is true or false, realistically they would just always deliver whatever behavior they're building for their custom app/plugin. I think I'm convinced that we can remove this sooner rather than later (meaning, at the earliest, after getsentry is updated and 21.12.0 goes out).

Hmm ... single-tenant, though ... 🤔

Yup, PR needed there as well!

Noted on getsentry/self-hosted#796. 👍

billyvg

Code wise this seems good, I think it'd be good to get a review from @getsentry/ecosystem too though

NisanthanNanthakumar

👍 changes to the sessionstack plugin look good!

wedamija

I think it would be wise to split out the FE/BE pieces of this pr. Deploy the BE piece first then the FE, that way we make sure we don't have any issues during deploy.

wedamija · 2021-12-09T20:09:24Z

src/sentry/conf/server.py

+# for now, but in the rest of _this_ codebase we should reference
+# SENTRY_SELF_HOSTED.
+SENTRY_ONPREMISE = True  # deprecated, use ...
+SENTRY_SELF_HOSTED = SENTRY_ONPREMISE  # ... this instead


Not too important for this pr, but if we want to remove SENTRY_ONPREMISE we should also check with the single tenant team to see whether they set this variable differently as well.

This reverts commit 31ad2a9.

chadwhitacre requested a review from a team as a code owner December 9, 2021 12:21

chadwhitacre requested review from a team December 9, 2021 12:21

chadwhitacre mentioned this pull request Dec 9, 2021

Rename onpremise to self-hosted getsentry/self-hosted#796

Closed

42 tasks

Base automatically changed from cwlw/remove-remote-newsletter to master December 9, 2021 12:22

chadwhitacre force-pushed the cwlw/rename-configs branch from ae97218 to ba0d0b4 Compare December 9, 2021 14:25

vercel bot deployed to Preview – sentry December 9, 2021 14:26 View deployment

vercel bot deployed to Preview – storybook December 9, 2021 14:26 View deployment

chadwhitacre commented Dec 9, 2021

View reviewed changes

vercel bot deployed to Preview – sentry December 9, 2021 16:07 View deployment

vercel bot deployed to Preview – storybook December 9, 2021 16:07 View deployment

chadwhitacre force-pushed the cwlw/rename-configs branch from 2f2fdc2 to ba0d0b4 Compare December 9, 2021 16:09

billyvg approved these changes Dec 9, 2021

View reviewed changes

NisanthanNanthakumar reviewed Dec 9, 2021

View reviewed changes

Rename onpremise to self-hosted in configs

ef80ddf

chadwhitacre force-pushed the cwlw/rename-configs branch from ba0d0b4 to ef80ddf Compare December 9, 2021 19:56

vercel bot deployed to Preview – sentry December 9, 2021 19:56 View deployment

vercel bot deployed to Preview – storybook December 9, 2021 19:56 View deployment

wedamija reviewed Dec 9, 2021

View reviewed changes

chadwhitacre merged commit 31ad2a9 into master Dec 9, 2021

chadwhitacre deleted the cwlw/rename-configs branch December 9, 2021 20:30

chadwhitacre added a commit that referenced this pull request Dec 9, 2021

Revert "Rename onpremise to self-hosted in configs (#30528)"

7a2a1a7

This reverts commit 31ad2a9.

chadwhitacre mentioned this pull request Dec 9, 2021

Revert "Rename onpremise to self-hosted in configs" #30550

Merged

chadwhitacre added a commit that referenced this pull request Dec 9, 2021

Revert "Rename onpremise to self-hosted in configs (#30528)" (#30550)

fb0c745

This reverts commit 31ad2a9.

This was referenced Dec 9, 2021

Rename to self-hosted in backend #30556

Closed

Rename to self-hosted in (most of) frontend #30558

Closed

Rename to self-hosted in SessionStack plugin #30559

Closed

github-actions bot locked and limited conversation to collaborators Dec 25, 2021

Uh oh!

Rename onpremise to self-hosted in configs #30528

Rename onpremise to self-hosted in configs #30528

Uh oh!

Conversation

chadwhitacre commented Dec 9, 2021

Uh oh!

github-actions bot commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

size-limit report

Uh oh!

chadwhitacre commented Dec 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chadwhitacre Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chadwhitacre Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billyvg left a comment

Choose a reason for hiding this comment

Uh oh!

NisanthanNanthakumar left a comment

Choose a reason for hiding this comment

Uh oh!

wedamija left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Dec 9, 2021 •

edited

Loading

chadwhitacre Dec 9, 2021 •

edited

Loading

chadwhitacre Dec 9, 2021 •

edited

Loading