Repository navigation
sourcegraph: Drop redundant PRECISE_CODE_INTEL_UPLOAD_* env - #992
Conversation
|
🤖 I'm an AI agent (Amp), posting on behalf of @bobheadxi. I tested this PR on a real cluster. Full details are in the Amp thread. Warning TL;DR: the description needs a fix before merging. The template change is correct, and default and documented setups are unaffected. But for deployments with The template change looks correct. Comparing
Upgrading a default install from One claim in the description and CHANGELOG is inaccurate. It's about deployments with
Upgrading the same Helm 3 release with Helm 4 does the same thing. The API server warns about this when the duplicates are created: The first upgrade doesn't change runtime behaviour, because the deleted values equal the built-in defaults. But the "check that bucket exists before upgrading" warning comes one upgrade too early. The switch happens on the next Suggested fix (text only, no template change): say in the description and CHANGELOG that in this setup, the |
The hardcoded PRECISE_CODE_INTEL_UPLOAD_BACKEND=blobstore and PRECISE_CODE_INTEL_UPLOAD_AWS_ENDPOINT=http://blobstore:9000 on frontend, worker, precise-code-intel-worker, and syntactic-code-intel-worker match Sourcegraph's built-in defaults, and the legacy compat shim ignores the endpoint when the backend is blobstore. SOURCEGRAPH_UPLOAD_* in *.env is now the only object storage configuration in the chart. They also rendered after *.env, so with blobstore.enabled they overrode any PRECISE_CODE_INTEL_UPLOAD_* a user set there. Follow-up to sourcegraph/deploy-sourcegraph-docker#1461. Amp-Thread-ID: https://ampcode.com/threads/T-01a19e9e-5e4f-7408-9834-0de841fee3e4 Co-authored-by: Amp <amp@ampcode.com>
Helm's three-way merge deletes every duplicate env entry on the upgrade that removes the chart's hardcoded values, so a *.env override only comes back on the following helm upgrade. Amp-Thread-ID: https://ampcode.com/threads/T-01a19e9e-5e4f-7408-9834-0de841fee3e4 Co-authored-by: Amp <amp@ampcode.com>
4ff85a6 to
0ef5b7b
Compare
Follow-up to sourcegraph/deploy-sourcegraph-docker#1461, same cleanup for the Helm chart.
Four templates still set
PRECISE_CODE_INTEL_UPLOAD_BACKEND=blobstoreandPRECISE_CODE_INTEL_UPLOAD_AWS_ENDPOINT=http://blobstore:9000under{{ if .Values.blobstore.enabled }}:frontend,worker,precise-code-intel-worker, andsyntactic-code-intel-worker. A customer reading the templates could reasonably think bothPRECISE_CODE_INTEL_UPLOAD_*andSOURCEGRAPH_UPLOAD_*need changing to move to S3 or GCS. This removes the legacy lines soSOURCEGRAPH_UPLOAD_*in*.envis the only object storage configuration in the chart, matching theexternal-object-storageexamples.The removed values did nothing:
PRECISE_CODE_INTEL_UPLOAD_BACKENDdefaults toblobstore(lsifuploadstore/config.go), and the endpoint defaults tohttp://blobstore:9000outside the single-container deployment (deploy/endpoints.go).blobstore,resolveLegacyCompatStorageuses the sharedSOURCEGRAPH_UPLOAD_*config and keeps only the legacy bucket name, so the legacy endpoint was never read.Impact on existing deployments
lsif-uploadsbucket.blobstore.enabled: falsewithSOURCEGRAPH_UPLOAD_*in*.env(the documented path): no change at all. The removed block never rendered for these deployments, so the manifests are byte-identical.blobstore.enabled: truewithSOURCEGRAPH_UPLOAD_BACKEND=S3|GCSin*.env: no change. Code-intel uploads already went to the shared bucket under thelsif-uploads/prefix via the compat shim, and still do.blobstore.enabled: truewithPRECISE_CODE_INTEL_UPLOAD_BACKEND=S3|GCSin*.env: behaviour fix, one upgrade later. The chart's hardcodedblobstorerendered after*.env, and Kubernetes takes the last duplicate env name, so these overrides were silently ignored and uploads went to the bundled blobstore. On the upgrade to this chart, Helm's three-way merge deletes every env entry with those names, the*.envoverride included, so the live Deployments carry neither value and runtime behaviour is unchanged (the deleted values equal the built-in defaults). On the nexthelm upgrade, even one with no changes, the override is rendered once, takes effect, and uploads switch to the dedicated bucket the customer configured. Before that second upgrade, make sure the bucket exists and is reachable, or move the settings toSOURCEGRAPH_UPLOAD_*, or remove them. Running a secondhelm upgradestraight away makes the switch happen at a known time. Existing uploads in the bundled blobstore are not migrated. Verified on a kind cluster by @bobheadxi's agent, see the comment below.Impact on new deployments
None beyond less to read. The
external-object-storageexamples already disable blobstore and set onlySOURCEGRAPH_UPLOAD_*.Test plan
helm lintandhelm unittest ./charts/sourcegraphpass (218 tests, 12 snapshots).helm templateoforigin/mainvs this branch with default values: the only difference is the two variables removed from the three services that render by default (syntactic-code-intel-workeris disabled by default).helm template --set frontend.env.PRECISE_CODE_INTEL_UPLOAD_BACKEND.value=S3onorigin/mainrendersS3followed by a secondblobstoreentry; on this branch it rendersS3once.