Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

batch_exec: inject docker container registry as a prefix #945

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

danieldides
Copy link

@danieldides danieldides commented Mar 1, 2023

Part of https://github.com/sourcegraph/customer/issues/1872
Depends on: https://github.com/sourcegraph/sourcegraph/pull/48499

This checks for the presence of an optional environment variable passed to the src batch exec directive that contains a docker registry URL. If it's present, it prefixes the container name with the registry information to create a fully-qualified domain name for all subsequent docker commands.

Test plan

Batch spec:

name: test
description: Add Hello World to READMEs

on:
  - repositoriesMatchingQuery: file:README.md count:4

steps:
  - run: IFS=$'\n'; echo Hello World | tee -a $(find -name README.md)
    container: busybox:1.35.0

changesetTemplate:
  title: Hello World
  body: My first batch change!
  commit:
    message: Append Hello World to all README.md files
  branch: ${{ batch_change.name }}

Add

for _, step := range task.Steps {
	fmt.Println("step: ", step.Container)
}

before the docker.NewImageCache() function call.

Execute a batch change without the environment variable set (default):

Screenshot 2023-03-01 at 14 03 54

Note the container name is just 1.35.0.

Execute a batch change with the environment variable set:

Restart sg start batches setting:

batches-executor:
    <<: *executor_template
    cmd: |
      env TMPDIR="$HOME/.sourcegraph/batches-executor-temp" .bin/executor
    env:
      EXECUTOR_QUEUE_NAME: batches
      EXECUTOR_MAXIMUM_NUM_JOBS: 8
      EXECUTOR_DOCKER_REGISTRY_PREFIX_URL: us-central1-docker.pkg.dev/control-plane-5e9ee072/remote-docker-hub

Or similar in sg.config.yaml. Restart the batch change (you may need to make a small change to it to prevent cached results from showing):
Screenshot 2023-03-01 at 14 29 57

Note the container name is now fully prefixed.

environ[pair[0]] = pair[1]
}

dockerPrefix, ok := environ["DOCKER_PREFIX_REGISTRY_URL"]
Copy link
Member

Choose a reason for hiding this comment

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

could we instead make this a flag that the server passes? Or do we want to configure this per-executor?
That way we wouldn't need to parse the env here.

if ok {
for i := 0; i < len(task.Steps); i++ {
step := &task.Steps[i]
step.Container = fmt.Sprintf("%s/%s", dockerPrefix, step.Container)
Copy link
Member

Choose a reason for hiding this comment

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

Changing the step might change the cache key, which which would case a mismatch between backend and src-cli 🤔 I'd need to validate that, but if that's the case we should only change it when we actually use the images, not on the step element itself.

Copy link
Member

Choose a reason for hiding this comment

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

If you want to validate that yourself, you'd want to run a spec with 2 steps, execute, change the 2nd step command or whatever, and see if a re-execution would hit a cache result for step1 and then if src-cli correctly sees it and skips step 1 in execution.

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

Successfully merging this pull request may close these issues.

2 participants