Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

feat(appliance): Change site-admin updates button to point to Appliance based on env var #64167

Merged
merged 7 commits into from
Jul 31, 2024
Merged

feat(appliance): Change site-admin updates button to point to Appliance based on env var #64167

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
db70bff
feat(appliance): change update endpoint based on env var
Chickensoupwithrice Jul 30, 2024
776fc29
misc: add type to storybook
Chickensoupwithrice Jul 30, 2024
bc30f4d
Merge branch 'main' into al/REL-300/point-update-if-appliance
Chickensoupwithrice Jul 30, 2024
0bb4551
fix(appliance): missed a file to add the type to
Chickensoupwithrice Jul 31, 2024
8a0cac4
Merge branch 'al/REL-300/point-update-if-appliance' of github.com:sou…
Chickensoupwithrice Jul 31, 2024
afa7a71
Merge branch 'main' into al/REL-300/point-update-if-appliance
Chickensoupwithrice Jul 31, 2024
89b7ed6
Suggested changes (squashme)
Jul 31, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions client/web/dev/utils/create-js-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export const createJsContext = ({ sourcegraphBaseUrl }: { sourcegraphBaseUrl: st
accessTokensExpirationDaysOptions: [7, 14, 30, 60, 90],
allowSignup: true,
batchChangesEnabled: true,
applianceManaged: false,
batchChangesDisableWebhooksWarning: false,
batchChangesWebhookLogsEnabled: true,
executorsEnabled: false,
Expand Down
4 changes: 4 additions & 0 deletions client/web/src/enterprise/site-admin/SiteAdminSidebar.story.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ export const AdminSidebarItems: StoryFn = () => (
batchChangesExecutionEnabled={true}
batchChangesWebhookLogsEnabled={true}
codeInsightsEnabled={true}
applianceManaged={false}
endUserOnboardingEnabled={false}
/>
<SiteAdminSidebar
Expand All @@ -44,6 +45,7 @@ export const AdminSidebarItems: StoryFn = () => (
batchChangesExecutionEnabled={true}
batchChangesWebhookLogsEnabled={true}
codeInsightsEnabled={true}
applianceManaged={false}
endUserOnboardingEnabled={false}
/>
<SiteAdminSidebar
Expand All @@ -54,6 +56,7 @@ export const AdminSidebarItems: StoryFn = () => (
batchChangesExecutionEnabled={false}
batchChangesWebhookLogsEnabled={false}
codeInsightsEnabled={true}
applianceManaged={false}
endUserOnboardingEnabled={false}
/>
<SiteAdminSidebar
Expand All @@ -64,6 +67,7 @@ export const AdminSidebarItems: StoryFn = () => (
batchChangesExecutionEnabled={true}
batchChangesWebhookLogsEnabled={true}
codeInsightsEnabled={false}
applianceManaged={false}
endUserOnboardingEnabled={false}
/>
</Grid>
Expand Down
1 change: 1 addition & 0 deletions client/web/src/integration/jscontext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ export const createJsContext = ({ sourcegraphBaseUrl }: { sourcegraphBaseUrl: st
accessTokensExpirationDaysOptions: [7, 30, 60, 90],
allowSignup: false,
batchChangesEnabled: true,
applianceManaged: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should maybe make this false for the integration test

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah that might even fix the failing test, but I agree we should do it anyway

batchChangesDisableWebhooksWarning: false,
batchChangesWebhookLogsEnabled: true,
codeInsightsEnabled: true,
Expand Down
5 changes: 5 additions & 0 deletions client/web/src/jscontext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,6 +196,11 @@ export interface SourcegraphContext extends Pick<Required<SiteConfiguration>, 'e

batchChangesWebhookLogsEnabled: boolean

/**
* Whether this sourcegraph instance is managed by Appliance
*/
applianceManaged: boolean

/**
* Whether Cody is enabled on this instance. Check
* {@link SourcegraphContext.codyEnabledForCurrentUser} to see whether Cody is enabled for the
Expand Down
1 change: 1 addition & 0 deletions client/web/src/routes.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,7 @@ export const routes: RouteObject[] = [
sideBarGroups={props.siteAdminSideBarGroups}
overviewComponents={props.siteAdminOverviewComponents}
codeInsightsEnabled={window.context.codeInsightsEnabled}
applianceManaged={window.context.applianceManaged}
telemetryRecorder={props.platformContext.telemetryRecorder}
/>
)}
Expand Down
4 changes: 4 additions & 0 deletions client/web/src/site-admin/SiteAdminArea.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export interface SiteAdminAreaRouteContext
overviewComponents: readonly React.ComponentType<React.PropsWithChildren<{}>>[]

codeInsightsEnabled: boolean
applianceManaged: boolean

endUserOnboardingEnabled: boolean
}
Expand All @@ -77,6 +78,7 @@ interface SiteAdminAreaProps
authenticatedUser: AuthenticatedUser
isSourcegraphDotCom: boolean
codeInsightsEnabled: boolean
applianceManaged: boolean
}

const sourcegraphOperatorSiteAdminMaintenanceBlockItems = new Set([
Expand Down Expand Up @@ -142,6 +144,7 @@ const AuthenticatedSiteAdminArea: React.FunctionComponent<React.PropsWithChildre
telemetryService: props.telemetryService,
telemetryRecorder: props.telemetryRecorder,
codeInsightsEnabled: props.codeInsightsEnabled,
applianceManaged: props.applianceManaged,
endUserOnboardingEnabled,
}

Expand All @@ -161,6 +164,7 @@ const AuthenticatedSiteAdminArea: React.FunctionComponent<React.PropsWithChildre
batchChangesExecutionEnabled={props.batchChangesExecutionEnabled}
batchChangesWebhookLogsEnabled={props.batchChangesWebhookLogsEnabled}
codeInsightsEnabled={props.codeInsightsEnabled}
applianceManaged={props.applianceManaged}
endUserOnboardingEnabled={endUserOnboardingEnabled}
/>
<div className="flex-bounded">
Expand Down
1 change: 1 addition & 0 deletions client/web/src/site-admin/SiteAdminSidebar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export interface SiteAdminSideBarGroupContext extends BatchChangesProps {
isSourcegraphDotCom: boolean
codeInsightsEnabled: boolean
endUserOnboardingEnabled: boolean
applianceManaged: boolean
}

export interface SiteAdminSideBarGroup extends NavGroupDescriptor<SiteAdminSideBarGroupContext> {}
Expand Down
7 changes: 7 additions & 0 deletions client/web/src/site-admin/sidebaritems.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,13 @@ const maintenanceGroup: SiteAdminSideBarGroup = {
{
label: maintenanceGroupUpdatesItemLabel,
to: '/site-admin/updates',
condition: ({ applianceManaged }) => !applianceManaged,
},
{
label: maintenanceGroupUpdatesItemLabel,
// TODO: change this to point the appliance service
to: 'http://localhost:8889/',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This URL is wrong and will need to change to point to the ingress for maintenance UI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(I'm not sure what it is?)

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

condition: ({ applianceManaged }) => applianceManaged,
},
{
label: 'Documentation',
Expand Down
2 changes: 2 additions & 0 deletions cmd/frontend/internal/app/jscontext/jscontext.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,7 @@ type JSContext struct {
CodeIntelRankingDocumentReferenceCountsEnabled bool `json:"codeIntelRankingDocumentReferenceCountsEnabled"`

CodeInsightsEnabled bool `json:"codeInsightsEnabled"`
ApplianceManaged bool `json:"applianceManaged"`
CodeIntelligenceEnabled bool `json:"codeIntelligenceEnabled"`
SearchContextsEnabled bool `json:"searchContextsEnabled"`
NotebooksEnabled bool `json:"notebooksEnabled"`
Expand Down Expand Up @@ -436,6 +437,7 @@ func NewJSContextFromRequest(req *http.Request, db database.DB) JSContext {
CodyRequiresVerifiedEmail: siteResolver.RequiresVerifiedEmailForCody(ctx),

CodeSearchEnabledOnInstance: codeSearchLicensed,
ApplianceManaged: conf.IsApplianceManaged(),

ExecutorsEnabled: conf.ExecutorsEnabled(),
CodeIntelAutoIndexingEnabled: conf.CodeIntelAutoIndexingEnabled(),
Expand Down
9 changes: 9 additions & 0 deletions internal/conf/computed.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,11 @@ package conf

import (
"encoding/hex"
"os"
Copy link
Contributor

Choose a reason for hiding this comment

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

CI is complaining that this file isn't go-fmt-ed

stdlog "log" //nolint:logging // TODO move all logging to sourcegraph/log
"net/url"
"slices"
"strconv"
"strings"
"time"

Expand Down Expand Up @@ -533,6 +535,13 @@ func AuthPrimaryLoginProvidersCount() int {
return c
}

func IsApplianceManaged() bool {
if v, _ := strconv.ParseBool(os.Getenv("APPLIANCE_MANAGED")); v {
return v
}
return false
}

// SearchSymbolsParallelism returns 20, or the site config
// "debug.search.symbolsParallelism" value if configured.
func SearchSymbolsParallelism() int {
Expand Down
Loading