Skip to content

Comments

fix: private gomod approach needs updating for mirror clones#148

Open
js-murph wants to merge 1 commit intomainfrom
johnm/improve-go-mod
Open

fix: private gomod approach needs updating for mirror clones#148
js-murph wants to merge 1 commit intomainfrom
johnm/improve-go-mod

Conversation

@js-murph
Copy link
Collaborator

What?

Updates the private go-mod fetcher to work with mirror clones. Additionally swaps from shelling out to create the zip to use https://pkg.go.dev/golang.org/x/mod/zip instead

Why?

The existing behaviour does not work with the new mirror clone approach.

@js-murph js-murph requested a review from a team as a code owner February 24, 2026 02:37
@js-murph js-murph requested review from nssherpa and removed request for a team February 24, 2026 02:37

// Local clone from the mirror — git hardlinks objects by default.
// #nosec G204 - repo.Path() and cloneDir are controlled by us
cmd := exec.CommandContext(ctx, "git", "clone", repo.Path(), cloneDir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wouldn't it be better to do git clone --branch ... ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Almost certainly yes, I will fix that.

@@ -284,21 +288,35 @@ func (p *privateFetcher) generateMod(ctx context.Context, repo *gitclone.Reposit
}

func (p *privateFetcher) generateZip(ctx context.Context, repo *gitclone.Repository, modulePath, version string) (io.ReadSeekCloser, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't recall when this is run, is it only if we don't already have a zip in the cache for the given version?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, at least that's what I recall I did. I'll double check.

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