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

feat(git): initialise submodules when cloning repos #4353

Merged
merged 1 commit into from Sep 25, 2019
Merged

feat(git): initialise submodules when cloning repos #4353

merged 1 commit into from Sep 25, 2019

Conversation

JamieMagee
Copy link
Contributor

@rarkins
Copy link
Collaborator

rarkins commented Aug 23, 2019

Question: Don't we need to prevent Renovate from scanning files within the submodules and attempting to update them?

@JamieMagee
Copy link
Contributor Author

Good question, it's not a scenario I've considered. I'll test later.

@viceice
Copy link
Member

viceice commented Aug 23, 2019

Maybe the submodule checkout should be an option, because you have to add an exclude rule for the submodule folders to renovate.

Maybe we can check for a .gitmodules file and extract the folders to exclude from there?

@rarkins
Copy link
Collaborator

rarkins commented Aug 23, 2019

I think the submodules are only required for artifact generation (lock files, etc), so potentially we can lazily fetch them as-needed. But if we fetch them prior to scanning/extracting then we ideally need a way to automatically exclude them. .gitmodules could be that way.

@rarkins
Copy link
Collaborator

rarkins commented Aug 24, 2019

There's one possibility I can think of where submodules might be useful during extraction. It's possible for some managers like Maven that a package could point to a "parent" package and that parent package includes information such as the default registry to use. In this case if we don't have submodules resolved then the package package would look like a broken local link. It's an edge case but maybe a valid one.

However, preventing problems during extraction could be pretty simple - we already call getFileList() from within gitfs, so it could pretty easily filter out submodule files at the same time.

Copy link
Collaborator

@rarkins rarkins left a comment

Choose a reason for hiding this comment

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

Make sure that the getFileList() function doesn't return submodule files to the upper layer which extracts packages

@JamieMagee
Copy link
Contributor Author

Sure, I'll see what the best way to do this is, with an eye on implementing #2502 next.

@JamieMagee
Copy link
Contributor Author

Make sure that the getFileList() function doesn't return submodule files to the upper layer which extracts packages

I've added the check to getFileList

Maybe the submodule checkout should be an option, because you have to add an exclude rule for the submodule folders to renovate.

I'm more of a fan of enabling it by default. I think it's a sensible default, and makes it easier for users to onboard to renovate.

@JamieMagee
Copy link
Contributor Author

@rarkins This is ready for review

@rarkins
Copy link
Collaborator

rarkins commented Sep 12, 2019

@JamieMagee hadn't you added code to exclude submodules files from the getFileList()? I don't see it now

@JamieMagee
Copy link
Contributor Author

Yeah, I even added tests. Looks like the branch I rebased was out of date with origin, so I pushed an old commit.

@rarkins
Copy link
Collaborator

rarkins commented Sep 12, 2019

Thanks. FYI, my position on rebasing is:

  • You don't need to do it, because we always merge squash to master anyway
  • I prefer you don't do it, because I have seen too many human errors because of it and I like to keep the history within the PR
  • But if you really prefer it, I don't object

@JamieMagee
Copy link
Contributor Author

Sure, I understand that the merging strategy is mostly a personal preference.

@rarkins rarkins merged commit e792268 into renovatebot:master Sep 25, 2019
@renovate-bot
Copy link
Collaborator

🎉 This PR is included in version 19.52.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@JamieMagee JamieMagee deleted the clone-submodules branch September 25, 2019 20:06
@ajcann
Copy link

ajcann commented Sep 26, 2019

@rarkins Are we able to exclude the submodule recurse from happening? Currently this breaks renovate for us as our git submodules belong to private repositories.

@JamieMagee
Copy link
Contributor Author

@ajcann Yeah, this should be fairly easy to add a config flag for.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Add an option to install git submodules before installing dependencies
5 participants