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

Ensure WORKERS is respected in syntect server image as env var #59018

Merged
merged 3 commits into from
Dec 15, 2023

Conversation

daxmc99
Copy link
Contributor

@daxmc99 daxmc99 commented Dec 14, 2023

Ensure CI passes

Test plan

bazel build //docker-images/syntax-highlighter:image --config=darwin-docker

@cla-bot cla-bot bot added the cla-signed label Dec 14, 2023
@sourcegraph-bot
Copy link
Contributor

sourcegraph-bot commented Dec 14, 2023

Codenotify: Notifying subscribers in CODENOTIFY files for diff 474813e...8ce7c4a.

Notify File(s)
@varungandhi-src docker-images/syntax-highlighter/BUILD.bazel

@daxmc99 daxmc99 requested a review from jhchabran December 14, 2023 23:01
# bad grammar/file combinations. If it happens with four workers, only 1/4th of
# requests will be affected for a short period of time. Each worker can require
# at peak around 1.1 GiB of memory.
"-workers=$WORKERS",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is WORKERS guaranteed to be set? If not, should we have a default value here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Checking this as part of the review:

~/work/sourcegraph  dax/update_syntect_server $ docker load --input=bazel-bin/docker-images/syntax-highlighter/image_tarball/tarball.tar
69635e4afa19: Loading layer [==================================================>]  23.22MB/23.22MB
9eb57e8e2f08: Loading layer [==================================================>]  14.48MB/14.48MB
Loaded image: syntect-server:candidate
~/work/sourcegraph  dax/update_syntect_server $ ba
~/work/sourcegraph  dax/update_syntect_server $ docker run --rm --entrypoint=/bin/sh -it syntect-server:candidate
WARNING: The requested image's platform (linux/amd64) does not match the detected host platform (linux/arm64/v8) and no specific platform was requested
/ # env
WORKERS=4

Copy link
Contributor

@jhchabran jhchabran left a comment

Choose a reason for hiding this comment

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

Manually checked the WORKERS env var being there by default, all good LGTM

@daxmc99 daxmc99 enabled auto-merge (squash) December 15, 2023 17:39
@daxmc99 daxmc99 merged commit e5e62a0 into main Dec 15, 2023
@daxmc99 daxmc99 deleted the dax/update_syntect_server branch December 15, 2023 17:48
@daxmc99
Copy link
Contributor Author

daxmc99 commented Dec 15, 2023

Minor improvement but this a bandaid instead of attempting https://github.com/sourcegraph/sourcegraph/issues/21942

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants