-
Notifications
You must be signed in to change notification settings - Fork 1.3k
add keep-git-dir and submodules controls for Git querystring URLs #6173
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
Conversation
Signed-off-by: Tonis Tiigi <[email protected]>
Signed-off-by: Tonis Tiigi <[email protected]>
if err != nil { | ||
return errors.Errorf("invalid submodules value: %q", v[0]) | ||
} | ||
gf.Submodules = &vv |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks
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)) | ||
} |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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
orfalse
- 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?
There was a problem hiding this comment.
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>
?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
Signed-off-by: Tonis Tiigi <[email protected]>
Added the valueless versions per #6173 (comment) |
<!--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]>
follow-up #6172
fixes #4974
@tianon