feat(appliance): Change site-admin updates button to point to Appliance based on env var #64167
Conversation
when APPLIANCE_MANAGED is set to true the site-admin page points to http://localhost:8889 When it is set to false, nothing changes. Mostly had to track down all the places in the config / types get set. The go backend generates these. The integration test defaults to false (though we should maybe have another one?)
…rcegraph/sourcegraph into al/REL-300/point-update-if-appliance
|
fuck i hate merge commits. how do i avoid merge commits? |
| { | ||
| label: maintenanceGroupUpdatesItemLabel, | ||
| // TODO: change this to point the appliance service | ||
| to: 'http://localhost:8889/', |
There was a problem hiding this comment.
This URL is wrong and will need to change to point to the ingress for maintenance UI.
There was a problem hiding this comment.
(I'm not sure what it is?)
There was a problem hiding this comment.
Currently, there is no separate ingress for the maintenance UI. Can we do same-domain / relative path redirects here? If so, /appliance/updates might be sensible. AFAIK the point of https://linear.app/sourcegraph/issue/REL-300/put-update-redirect-into-current-sourcegraph-admin-panel is to put a "foothold" into site-admin so that when we later implement appliance UI-driven updates, they can work without a code search upgrade first (presumably configmap-driven).
We can figure out the exact ingress dance later: presumably, we'll need to add a route rule to direct HTTP requests to paths starting with /appliance to an appliance-frontend service (which doesn't exist yet), rather than the sourcegraph-frontend service. Basically, I think the routing logic for when code search suite is healthy and when it's unhealthy are totally disjoint. The latter is implemented in appliance/healthchecker service selector flipping, and the former is what we're laying a foothold for here. Any routing changes to the lone ingress, and any additional k8s Services required to support UI-driven upgrades, can be provisioned by the appliance. We don't need to write that code just yet, because we can couple it to a self-update in the near future.
There's always a chance we'll get this wrong and that our first upgrade will have to be configmap-driven, but this is the best we can do I think. WDYT?
| @@ -27,6 +27,7 @@ export const createJsContext = ({ sourcegraphBaseUrl }: { sourcegraphBaseUrl: st | |||
| accessTokensExpirationDaysOptions: [7, 30, 60, 90], | |||
| allowSignup: false, | |||
| batchChangesEnabled: true, | |||
| applianceManaged: true, | |||
There was a problem hiding this comment.
We should maybe make this false for the integration test
There was a problem hiding this comment.
Yeah that might even fix the failing test, but I agree we should do it anyway
There was a problem hiding this comment.
Awesome to see! I left one comment on the appliance URL thread, and also we need to fix up bazel test //client/web:test. Probably some test setup needs our new parameter. We can just pass false for now since nothing uses the new code path yet.
We should get this reviewed by a recent contributor to site-admin, probably.
internal/conf/computed.go
Outdated
| @@ -2,9 +2,11 @@ package conf | |||
|
|
|||
| import ( | |||
| "encoding/hex" | |||
| "os" | |||
There was a problem hiding this comment.
CI is complaining that this file isn't go-fmt-ed
| { | ||
| label: maintenanceGroupUpdatesItemLabel, | ||
| // TODO: change this to point the appliance service | ||
| to: 'http://localhost:8889/', |
There was a problem hiding this comment.
Currently, there is no separate ingress for the maintenance UI. Can we do same-domain / relative path redirects here? If so, /appliance/updates might be sensible. AFAIK the point of https://linear.app/sourcegraph/issue/REL-300/put-update-redirect-into-current-sourcegraph-admin-panel is to put a "foothold" into site-admin so that when we later implement appliance UI-driven updates, they can work without a code search upgrade first (presumably configmap-driven).
We can figure out the exact ingress dance later: presumably, we'll need to add a route rule to direct HTTP requests to paths starting with /appliance to an appliance-frontend service (which doesn't exist yet), rather than the sourcegraph-frontend service. Basically, I think the routing logic for when code search suite is healthy and when it's unhealthy are totally disjoint. The latter is implemented in appliance/healthchecker service selector flipping, and the former is what we're laying a foothold for here. Any routing changes to the lone ingress, and any additional k8s Services required to support UI-driven upgrades, can be provisioned by the appliance. We don't need to write that code just yet, because we can couple it to a self-update in the near future.
There's always a chance we'll get this wrong and that our first upgrade will have to be configmap-driven, but this is the best we can do I think. WDYT?
Rebase instead. This can be done either by 
You'd have to unmake the merge with main to rebase from now on though, since you've already merged main in once. Not worth it IMO, let's just squash-merge this. |
|
@Chickensoupwithrice and I are pairing, pushed a few changes |
…ce based on env var (#64167) <!-- PR description tips: https://www.notion.so/sourcegraph/Write-a-good-pull-request-description-610a7fd3e613496eb76f450db5a49b6e --> This PR resolves [REL-300](https://linear.app/sourcegraph/issue/REL-300/put-update-redirect-into-current-sourcegraph-admin-panel). We point the Update button in the SiteAdmin sidebar to point to a different URL (Currently appliance localhost port, needs to be changed) based on the `APPLIANCE_MANAGED` env var. Most of the PR is tracking types / config down to the backend. There, a simple function checks for the existence of this env var and if it exists returns it's value. I may have updated extra unnecessary types (not certain) but I was following the compiler. Second commit is just updating the storybook type values We'll need another PR to the Helm chart to activate the env var once we want to switch people over to pointing to the appliance maintenance UI. @DaedalusG brought up the good point that even when managed by Appliance, the Upgrades page still provides valuable information to administrators, and so we may or may not actually want to leave this the way it is. TODO: - [ ] Change the URL that is pointed to when the env var is active (listed in a comment) <!-- REQUIRED; info at https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles --> Tested manually <!-- OPTIONAL; info at https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c --> - **feat(appliance): change update endpoint based on env var** - **misc: add type to storybook** --------- Co-authored-by: Craig Furman <[email protected]>
This PR resolves REL-300. We point the Update button in the SiteAdmin sidebar to point to a different URL (Currently appliance localhost port, needs to be changed) based on the
APPLIANCE_MANAGEDenv var.Most of the PR is tracking types / config down to the backend. There, a simple function checks for the existence of this env var and if it exists returns it's value.
I may have updated extra unnecessary types (not certain) but I was following the compiler.
Second commit is just updating the storybook type values
We'll need another PR to the Helm chart to activate the env var once we want to switch people over to pointing to the appliance maintenance UI.
@DaedalusG brought up the good point that even when managed by Appliance, the Upgrades page still provides valuable information to administrators, and so we may or may not actually want to leave this the way it is.
TODO:
Test plan
Tested manually
Changelog