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

Allow file fetchers to opt into loading git submodules #5982

Merged
merged 4 commits into from
Oct 27, 2022

Conversation

bdragon
Copy link
Contributor

@bdragon bdragon commented Oct 27, 2022

Context

Closes #5975

Repos are always git-cloned with --no-recurse-submodules. This can be problematic if a dependency lives in a git submodule and dependency resolution requires reading from it. For my use case, this is showing up when using go modules.

What's Changing

This PR adds #recurse_submodules_when_cloning? to Dependabot::FileFetchers::Base. If it returns a truthy value, repos are git-cloned with --recurse-submodules and --shallow-submodules; if it returns a falsy value, --no-recurse-submodules is used (the current behavior).

The default implementation of the method returns false, preserving the existing behavior for all file fetchers. Subclasses of Dependabot::FileFetchers::Base may override to opt into the behavior, and this PR does so for the go modules file fetcher.

The change also extends the behavior to the git-fetch and git-reset operations if source.commit is present (i.e., for testing). The relevant options used are git fetch --recurse-submodules=on-demand and git reset --recurse-submodules.

How to Review

I recommend reviewing this PR by commit:

0b1f097: I noticed that Dependabot::FileFetchers::Base declares some methods "private" (ruby private, prefixed with underscore) under a comment heading that says they should not be used by subclasses, but some "protected" (ruby private, no underscore) methods were mixed in with these under the same comment. This commit moves the protected methods above the comment, which makes the overall diff appear larger than what's actually changing.

41bac21: adds #recurse_submodules_when_cloning? and integrates it into #_clone_repo_contents.

2f8a624: overrides #recurse_submodules_when_cloning? for the go_modules file fetcher in order to opt into the behavior.

Private methods were interspersed with protected methods under comment heading, 'INTERNAL METHODS (not for use by sub-classes)'. This change simply moves the protected methods above this heading.
@bdragon bdragon requested a review from a team as a code owner October 27, 2022 00:08
@bdragon bdragon requested review from jakecoffman and jurre October 27, 2022 15:34
Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

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

Makes sense to me, great work!

@bdragon bdragon merged commit 5880c3c into main Oct 27, 2022
@bdragon bdragon deleted the bdragon/clone-recurse-submodules branch October 27, 2022 20:37
@pavera pavera mentioned this pull request Oct 31, 2022
@thepwagner
Copy link
Contributor

👋 hey friends! This broke our updates 😿

We build a go_modules app that uses a git submodule for some non-code resources. This submodule is irrelevant to our updates.
The submodule repository is owned by a different GitHub organization, so providing a token with appropriate access is inconvenient (e.g. requires a Personal Access Token?)

Before this PR, dependabot worked great.
After this PR was deployed, Dependabot can no longer clone our go_modules repository and therefore can not give us any updates.

Is there any way for us to disable this new functionality via configuration?

@@ -47,6 +47,10 @@ def go_mod
def go_sum
@go_sum ||= fetch_file_if_present("go.sum")
end

def recurse_submodules_when_cloning?
true
Copy link
Member

Choose a reason for hiding this comment

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

Something we can try here to mitigate the issue @thepwagner brought up is only install submodules if the initial dependency resolution step failed to run

We do something of that sorts for Terraform modules in

rescue SharedHelpers::HelperSubprocessFailed => e
if @retrying_lock && e.message.match?(MODULE_NOT_INSTALLED_ERROR)
mod = e.message.match(MODULE_NOT_INSTALLED_ERROR).named_captures.fetch("mod")
raise Dependabot::DependencyFileNotResolvable, "Attempt to install module #{mod} failed"
end
raise if @retrying_lock || !e.message.include?("terraform init")
# NOTE: Modules need to be installed before terraform can update the lockfile
@retrying_lock = true
run_terraform_init
retry
end

Copy link
Member

Choose a reason for hiding this comment

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

Another option might be we leave the submodule clone by default, but ignore any errors to attempt the update anyway? That way if it didn't matter then we still do the update.

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.

Enable fetching git submodules in cloning file fetchers
4 participants