build: Use syntax highlighter from locally built Docker image

[varungandhi-src]

Fixes https://github.com/sourcegraph/sourcegraph/issues/58395

Using the latest image by default should help reduce the number of regressions
as one tests changes locally and it is easy to forget that the latest version
of the highlighter is not used by sg start.

Test plan

Run sg start, navigate code.

There is a bug on main which will be reverted in https://github.com/sourcegraph/sourcegraph/pull/60668
Highlighting is broken without that fix.

Preview 🤩

Preview Link

[keegancsmith]

commandSets that depend on syntax-highlighter need to specify bazel as a dependency now

[keegancsmith]

TBH if we are using bazel here, we should just be using the binary it builds instead of docker? If we want to catch issues with the docker image we should add some sort of image test.

[varungandhi-src]

TBH if we are using bazel here, we should just be using the binary it builds instead of docker? If we want to catch issues with the docker image we should add some sort of image test.

I'm inclined to keep the Docker image around just for similarity to a real environment.

However, I agree with your point about image tests, I didn't realize those are easy to add nowadays. I will add one.

[keegancsmith]

The only place we are using docker in our whole devenv is for syntax highlighter. This is a relic of the past because it was difficult to build rust before bazel. I would say we should be using the new binary. I would let @jhchabran weigh in here, but my preference would be to get rid of docker.

[varungandhi-src]

The only place we are using docker in our whole devenv is for syntax highlighter.

Do you mean specifically for the search platform team or generally? On the graph team, we often end up using the enterprise-codeintel preset.

$ cat sg.config.yaml | rg 'docker load'
      docker load --input $(bazel cquery //cmd/executor-kubernetes:image_tarball --output=files)
      docker load --input $(bazel cquery //cmd/batcheshelper:image_tarball --output=files)
      docker load --input $(bazel cquery //docker-images/grafana:image_tarball --output=files)
      docker load --input $(bazel cquery //docker-images/prometheus:image_tarball --output=files)
      docker load --input $(bazel cquery //docker-images/postgres_exporter:image_tarball --output=files)
      docker load --input $(bazel cquery //docker-images/opentelemetry-collector:image_tarball --output=files)

Out of these, prometheus and grafana are part of enterprise-codeintel

[varungandhi-src]

Issue for adding image tests:

• https://github.com/sourcegraph/sourcegraph/issues/60690

[varungandhi-src]

commandSets that depend on syntax-highlighter need to specify bazel as a dependency now

Fixed in new commit: https://github.com/sourcegraph/sourcegraph/pull/60669/commits/d54abd86c5ac1bd6ea85495d9cf104f75593cd7c

It seems like a bunch of existing commandsets were already implicitly depending on bazel, but nobody must've complained because it is installed on everyone's machines now.

[jhchabran]

I'm inclined to keep the Docker image around just for similarity to a real environment.

Is that really what matters here? I mean we don't do that anywhere else, and that's what the container structure tests + e2e tests are for.

I checked the container we build for syntax-highlighter, to see if we're doing anything particular, and all we do there is setting up http-stabilizer and adding a prometheus tag, on top of of course adding the binary to the base image.

sg.config.yaml is pretty flexible and I understand the convenience of having the container being run as part of of the enterprise-codeintel command set, as I suppose you use graphana regularly to observe how things are running, so in that case, why don't we split the syntax-highlighter into two variants?

One for the common commandsets, that just run the binary locally, as it works nicely now as @keegancsmith pointed out, and not having to handle a docker container is simpler, whereas on the code intel command set, you'd use a syntax-highlighter-image which would be the one we have currently in the code?

The way I see this, the graph team simply uses whatever they prefer (that command set belongs to you folks), and everyone has the simple version.

[varungandhi-src]

I'm inclined to keep the Docker image around just for similarity to a real environment.

Is that really what matters here? I mean we don't do that anywhere else, and that's what the container structure tests + e2e tests are for.

I'd say people perform more interactions in their dev environment compared to what we are exercising in tests, so I'd say it's useful. But I don't think it's that big of a deal, especially the Docker image vs the binary. The vast majority of problems should be shared in both configurations.

The other thing is, I just noticed that we've actually disabled stdout and stderr for the highlighter (perhaps because of logspam), so I've filed an issue so that we can remove the logspam and turn these on, so that issues are more clearly visible.

• https://github.com/sourcegraph/sourcegraph/issues/60769

as I suppose you use graphana regularly to observe how things are running

To clarify, I'm not using Grafana much at all. My point was that Grafana is being used via Docker, so Keegan's point "The only place we are using docker in our whole devenv is for syntax highlighter" didn't seem quite right (or it's possible that I was misunderstanding his point -- since there are different command sets, "our whole devenv" technically depends on the person, as different people use different command sets day-to-day). So I thought that even if we got rid of requiring Docker for syntax-highlighter, it would still be needed anyways (because of Grafana etc.)

---

Overall, I don't think it is worth splitting syntax-highlighter into two different commands, one with Docker and one without. I will remove the Docker dependency as requested.

[jhchabran]

@varungandhi-src btw, you might want to check https://github.com/sourcegraph/sourcegraph/pull/61140 :)