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

Set EnvVar for Submodule mirrors #2512

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

tmcg-gusto
Copy link

A quick attempt at solving this issue. #2511

TL;DR: The agent doesn't provide an env var containing the directories of submodule git mirrors, so plugins (docker, in my case) can't mount those directories, causing git issues in containers.

Let me know if I missed anything obvious.

For testing, I've been struggling to get tests to pass locally (both on main, and on my branch). Do I need to be using a valid buildkite agent token in order to run tests? It looks like it's failing to send real data off to buildkite's API. Once I get the tests working I can add an expectation in the checkout integration test.

Copy link
Contributor

@triarius triarius left a comment

Choose a reason for hiding this comment

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

Thanks for adding this @tmcg-gusto. I have some opinions about the design. LMK what you think.

@@ -642,6 +645,10 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
}
}

for i, dir := range submoduleMirrorDirs {
e.shell.Env.Set(fmt.Sprintf("BUILDKITE_REPO_SUBMODULE_MIRROR_%d", i), dir)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to have 1 env var and a comma separated list as its value? I suspect its easier to get one env variable and parse the comma separated list than to search for all the the env variables containing the submodule mirror dirs. I know the docker plugin is in bash, which makes parsing the value hard, but it should be possible: https://stackoverflow.com/questions/27702452/loop-through-a-comma-separated-shell-variable

Copy link
Author

Choose a reason for hiding this comment

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

@triarius Yeah we could definitely do that, and that'd likely be easier to work with.

My goal for using the integer suffix was to keep things consistent with how it's done for arrays of configs in plugins. I imagine many plugins already have patterns for consuming those sort of environment variables (I know I've written it in a few plugins myself).

If y'all would prefer the comma separated list since we can safely assume there won't be commas in the mirror paths I'll make the adjustments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @tmcg-gusto - apologies it's taken some time to get a reply to you! I'd agree with @triarius that a comma-separated list in a single env var would be appropriate here. Would you mind making the adjustment then we can get this merged in?

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.

3 participants