Skip to content

Conversation

tonistiigi
Copy link
Member

@tonistiigi tonistiigi commented Aug 29, 2025

follow-up #6172
fixes #4974

@tianon

if err != nil {
return errors.Errorf("invalid submodules value: %q", v[0])
}
gf.Submodules = &vv
Copy link
Member

@AkihiroSuda AkihiroSuda Aug 29, 2025

Choose a reason for hiding this comment

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

For boolean values, probably value-less query (v[0] == "") can be allowed

https://example.com/foo.git?keep-git-dir

Copy link
Member

@AkihiroSuda AkihiroSuda left a comment

Choose a reason for hiding this comment

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

Thanks

Comment on lines +649 to +653
if !gs.src.SkipSubmodules {
_, err = git.Run(ctx, "submodule", "update", "--init", "--recursive", "--depth=1")
if err != nil {
return nil, errors.Wrapf(err, "failed to update submodules for %s", urlutil.RedactCredentials(gs.src.Remote))
}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe that's an edge case but was wondering if we could have submodules being either a bool to enable/disable init submodules but also a list to init some of them only like submodules=./sub1,./sub2?

Copy link
Member Author

Choose a reason for hiding this comment

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

Possible problems:

  • more idiomatic querystring would be submodules=./sub1&submodules=./sub2
  • what if submodule path is true or false
  • should this support pattern matching

Because of this would make more sense as a different builtin key that can be added later in a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

Because of this would make more sense as a different builtin key that can be added later in a follow-up?

Mostly looking at reusing the same key but the query form you suggest submodules=./sub1&submodules=./sub2 makes more sense. So yeah we might need another key.

What do you think of init-submodules=<true|false> default to true and another one filter-submodules=<pattern>?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, was thinking smth like submodules-filter. Don't know if we would need init- prefix

Copy link
Member

Choose a reason for hiding this comment

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

Yes submodules=<bool> and submodules-filter=<pattern> LGTM

@tonistiigi
Copy link
Member Author

Added the valueless versions per #6173 (comment)

@tonistiigi tonistiigi merged commit 7ff02fb into moby:master Aug 29, 2025
225 of 229 checks passed
aevesdocker added a commit to docker/docs that referenced this pull request Sep 11, 2025
<!--Delete sections as needed -->

## Description

<!-- Tell us what you did and why -->

BuildKit v0.24 introduces URL like
`https://github.com/example/example.git?tag=v0.0.1&checksum=deadbeef`

## Related issues or tickets

<!-- Related issues, pull requests, or Jira tickets -->

- moby/buildkit#6172
- moby/buildkit#6173

## Reviews

<!-- Notes for reviewers here -->
<!-- List applicable reviews (optionally @tag reviewers) -->

- [X] Technical review
- [X] Editorial review
- [ ] Product review

---------

Signed-off-by: Akihiro Suda <[email protected]>
Signed-off-by: CrazyMax <[email protected]>
Co-authored-by: CrazyMax <[email protected]>
Co-authored-by: Allie Sadler <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Option to disable submodules / recursive clone
3 participants