appliance: deploy precise-code-intel#62624
Conversation
There was a problem hiding this comment.
On the one hand, it's probably clutter, on the other hand, establishing that it's definitely unused is work. Maybe we ask the team that maintains precise-code-intel if this is really indeed unused? (which team is that btw?)
There was a problem hiding this comment.
There are a number of cases like this across our services unfortunately, whereas I started wrapping these as an OKR for next quarter under "Sourcegraph service consistency".
TLDR: Leave it in for now, and I think we make an effort to come back and tackle these all at once along with until nits mentioned in the issue.
There was a problem hiding this comment.
There's maybe a bit of a footgun introduced by this PR's addition of ContainerConfig.EnvVars as a standard config feature (not specific to precise-code-intel): any user-configured env vars are already present in ctr.Env, returned by NewContainer() above. If you overwrite this field instead of appending, those vars will be lost.
If we wrote golden tests for every service that uses StandardConfig, ensuring they do not clobber user-defined env vars, we'd catch such errors, but that would defeat the whole point of standard config features (that they don't need to be endlessly re-tested and re-implemented by each service definition).
In some other languages, we could express this constraint in code by making EnvVars an immutable field (but the underlying list would be mutable, supporting appending or overwriting individual elements). We could write a linter, but I'm not sure the juice would be worth the squeeze.
There was a problem hiding this comment.
I handles something like this in the POC with options that would guard appending to certain fields, not allowing overwrites in certain cases: https://sourcegraph.sourcegraph.com/github.com/sourcegraph/sourcegraph-operator/-/blob/internal/controller/resource/pod/pod_template.go?L53
Could we use something like this again?
There was a problem hiding this comment.
Those builders are a nudge in the right direction on this particular item, but they don't really guard against it. The factory functions (NewXYZ) accept options functions, but then they return structs. Nothing stops callers from mutating those struct fields. IMO returning to that builder pattern would be another case of the juice not being worth the squeeze (sorry to use this metaphor twice in one thread 😉 ), since it would add a lot of error-prone, test-necessitating indirection for not a whole lot of gain on this front (again, just IMO 🙏).
The only hard-and-fast way to do this that I can think of is to put an interface in front of the return value to make certain operations illegal. But that would be a ton of work (juice, squeeze again), and we'd have to add accessors for every field we do want to allow mutations on.
Stepping back, and I do hesitate to suggest this, but I do think this is a case of "let's be mindful and try to catch it at review time". WDYT?
There was a problem hiding this comment.
Those builders are a nudge in the right direction on this particular item, but they don't really guard against it.
Very true. I'm okay with the "let's be mindful" approach and if it becomes a problem further down the line we can take more "drastic" measures 😃
There was a problem hiding this comment.
Sounds good to me! 2-way door, as with many decisions 🧘♂️
There was a problem hiding this comment.
There was a problem hiding this comment.
Annotated output of:
go run ./internal/appliance/dev/compare-helm \
-component precise-code-intel \
-golden-file internal/appliance/testdata/golden-fixtures/precise-code-intel/default.yaml \
-helm-template-extra-args '--set preciseCodeIntel.serviceAccount.create=true'
Nothing stands out to me, but more eyes are very welcome of course!
2c2
< # helm: ServiceAccount/precise-code-intel-worker
---
> # golden: ServiceAccount/precise-code-intel-worker
5a6,8
> annotations:
> appliance.sourcegraph.com/configHash: 05b6b32b7f7702146178081817670b43a900fd5b2ed61dcdaa8e34d7a4e0d204
> creationTimestamp: "2024-04-19T00:00:00Z"
7,8d9
< app.kubernetes.io/component: precise-code-intel
< category: rbac
10a12,21
> namespace: NORMALIZED_FOR_TESTING
> ownerReferences:
> - apiVersion: v1
> blockOwnerDeletion: true
> controller: true
> kind: ConfigMap
> name: sg
> uid: NORMALIZED_FOR_TESTING
> resourceVersion: NORMALIZED_FOR_TESTING
> uid: NORMALIZED_FOR_TESTING
12c23
< # helm: Service/precise-code-intel-worker
---
> # golden: Service/precise-code-intel-worker
16a28
> appliance.sourcegraph.com/configHash: 05b6b32b7f7702146178081817670b43a900fd5b2ed61dcdaa8e34d7a4e0d204
18a31
> creationTimestamp: "2024-04-19T00:00:00Z"
21c34
< app.kubernetes.io/component: precise-code-intel
---
> app.kubernetes.io/component: precise-code-intel-worker
23a37,46
> namespace: NORMALIZED_FOR_TESTING
> ownerReferences:
> - apiVersion: v1
> blockOwnerDeletion: true
> controller: true
> kind: ConfigMap
> name: sg
> uid: NORMALIZED_FOR_TESTING
> resourceVersion: NORMALIZED_FOR_TESTING
> uid: NORMALIZED_FOR_TESTING
24a48,54
> clusterIP: NORMALIZED_FOR_TESTING
> clusterIPs:
> - NORMALIZED_FOR_TESTING
> internalTrafficPolicy: Cluster
> ipFamilies:
> - IPv4
> ipFamilyPolicy: SingleStack
27a58
> protocol: TCP
30a62
> protocol: TCP
34,35c66
< app.kubernetes.io/instance: release-name
< app.kubernetes.io/name: sourcegraph
---
> sessionAffinity: None
36a68,69
> status:
> loadBalancer: {}
38c71
< # helm: Deployment/precise-code-intel-worker
---
> # golden: Deployment/precise-code-intel-worker
43c76,78
< description: Handles conversion of uploaded precise code intelligence bundles.
---
> appliance.sourcegraph.com/configHash: 05b6b32b7f7702146178081817670b43a900fd5b2ed61dcdaa8e34d7a4e0d204
> creationTimestamp: "2024-04-19T00:00:00Z"
> generation: 1
45,47c80
< app.kubernetes.io/component: precise-code-intel
< app.kubernetes.io/instance: release-name
< app.kubernetes.io/managed-by: Helm
---
> app.kubernetes.io/component: precise-code-intel-worker
49c82
< app.kubernetes.io/version: 5.3.2
---
> app.kubernetes.io/version: 5.3.9104
51d83
< helm.sh/chart: sourcegraph-5.3.2
52a85,94
> namespace: NORMALIZED_FOR_TESTING
> ownerReferences:
> - apiVersion: v1
> blockOwnerDeletion: true
> controller: true
> kind: ConfigMap
> name: sg
> uid: NORMALIZED_FOR_TESTING
> resourceVersion: NORMALIZED_FOR_TESTING
> uid: NORMALIZED_FOR_TESTING
54a97
> progressDeadlineSeconds: 600
60,61d102
< app.kubernetes.io/instance: release-name
< app.kubernetes.io/name: sourcegraph
70a112
> creationTimestamp: null
73,74d114
< app.kubernetes.io/instance: release-name
< app.kubernetes.io/name: sourcegraph
75a116
> name: precise-code-intel-worker
77d117
< affinity: null
84a125
> apiVersion: v1
86,89d126
< - name: PRECISE_CODE_INTEL_UPLOAD_BACKEND
< value: blobstore
< - name: PRECISE_CODE_INTEL_UPLOAD_AWS_ENDPOINT
< value: http://blobstore:9000See golden fixture "with-blobstore" to see these vars added under certain
configs. I chose to comment in this PR with the default diff because the
with-blobstore diff contains all the resources from blobstore too, which is
visually noisy.
92a130
> apiVersion: v1
98a137
> failureThreshold: 3
103a143,144
> periodSeconds: 10
> successThreshold: 1
108a150
> protocol: TCP
110a153
> protocol: TCP
111a155
> failureThreshold: 3
116a161
> successThreshold: 1
129a175
> terminationMessagePath: /dev/termination-log
134,137c180,188
< nodeSelector: null
< securityContext: {}
< serviceAccountName: precise-code-intel-worker
< tolerations: null
---
> dnsPolicy: ClusterFirst
> restartPolicy: Always
> schedulerName: default-scheduler
> securityContext:
> fsGroup: 101
> fsGroupChangePolicy: OnRootMismatch
> runAsGroup: 101
> runAsUser: 100
> terminationGracePeriodSeconds: 30
140a192,259
> status: {}
> ---
> # golden: ConfigMap/sg
> apiVersion: v1
> data:
> spec: |
> spec:
> requestedVersion: "5.3.9104"
>
> blobstore:
> disabled: true
>
> codeInsights:
> disabled: true
>
> codeIntel:
> disabled: true
>
> frontend:
> disabled: true
>
> gitServer:
> disabled: true
>
> indexedSearch:
> disabled: true
>
> indexedSearchIndexer:
> disabled: true
>
> pgsql:
> disabled: true
>
> postgresExporter:
> disabled: true
>
> preciseCodeIntel: {}
>
> redisCache:
> disabled: true
>
> redisStore:
> disabled: true
>
> repoUpdater:
> disabled: true
>
> searcher:
> disabled: true
>
> symbols:
> disabled: true
>
> syntectServer:
> disabled: true
>
> worker:
> disabled: true
> kind: ConfigMap
> metadata:
> annotations:
> appliance.sourcegraph.com/currentVersion: 5.3.9104
> appliance.sourcegraph.com/managed: "true"
> creationTimestamp: "2024-04-19T00:00:00Z"
> name: sg
> namespace: NORMALIZED_FOR_TESTING
> resourceVersion: NORMALIZED_FOR_TESTING
> uid: NORMALIZED_FOR_TESTINGThere was a problem hiding this comment.
Nothing looks off to me either
a0a378f to
c39634f
Compare
Add standard feature for container env vars.
c39634f to
7f98eca
Compare
Since introducing the standard feature for container env vars in https://github.com/sourcegraph/sourcegraph/pull/62624, some test flakiness has been introduced. This is because container.go was iterating through an unsorted map, which made golden file assertions non-deterministic. This is fixed by sorting the map keys when appending user-provided env vars.
…62732) Since introducing the standard feature for container env vars in https://github.com/sourcegraph/sourcegraph/pull/62624, some test flakiness has been introduced. This is because container.go was iterating through an unsorted map, which made golden file assertions non-deterministic. This is fixed by sorting the map keys when appending user-provided env vars.
Add standard feature for container env vars.
Closes https://github.com/sourcegraph/sourcegraph-operator/issues/43.
Test plan
Golden tests included.