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.

Sign up for GitHub

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
Open

Set EnvVar for Submodule mirrors #2512

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions internal/job/checkout.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,6 +600,7 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
e.shell.Warningf("Failed to enumerate git submodules: %v", err)
} else {
mirrorSubmodules := e.ExecutorConfig.GitMirrorsPath != ""
submoduleMirrorDirs := make([]string, 0)
for _, repository := range submoduleRepos {
submoduleArgs := append([]string(nil), args...)
// submodules might need their fingerprints verified too
Expand All @@ -617,6 +618,8 @@ func (e *Executor) defaultCheckoutPhase(ctx context.Context) error {
return fmt.Errorf("getting/updating mirror dir for submodules: %w", err)
}

submoduleMirrorDirs = append(submoduleMirrorDirs, mirrorDir)

// Switch back to the checkout dir, doing other operations from GitMirrorsPath will fail.
if err := e.createCheckoutDir(); err != nil {
return fmt.Errorf("creating checkout dir: %w", err)
Expand All @@ -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?


if !mirrorSubmodules {
args = append(args, "submodule", "update", "--init", "--recursive", "--force")
if err := e.shell.Run(ctx, "git", args...); err != nil {
Expand Down