Ensure git submodules aren't interacted with if no-submodules is configured#3289
Ensure git submodules aren't interacted with if no-submodules is configured#3289
Conversation
patrobinson
left a comment
patrobinson
left a comment
There was a problem hiding this comment.
Some minor feedback, I think this is good to go otherwise
|
|
||
| // Git clean prior to checkout, we do this even if submodules have been | ||
| // disabled to ensure previous submodules are cleaned up | ||
| gitSubmodules := false |
There was a problem hiding this comment.
The comment above this is no longer true. I can see it was added in #1054 but there's no more information about why it was added.
There was a problem hiding this comment.
🤦🏼 I just read the comment and realised why this happened in the first place now.
Now I'm wondering what circumstances lead to this being the case, and how we could possibly cater for both situations.
There was a problem hiding this comment.
AIUI, unless it's a clean checkout (directory doesn't exist, it was empty, or the agent is configured to always do clean checkouts), the agent is running the checkout against a directory containing code that was checked out in a previous job. So it might contain submodules, that have since been removed in whatever ref is being checked out now.
So I wonder if we can harden this in some other way? Such as making hasGitSubmodules more robust, or skipping submodule cleaning if the checkout itself is clean?
Co-authored-by: Patrick Robinson <[email protected]>
patrobinson
left a comment
patrobinson
left a comment
There was a problem hiding this comment.
I'm happy with this, but it'd be good to get @DrJosh9000's feedback. I can't think of any negative consequences to not cleaning up old submodules but 🤷
|
There's some additional context here that we've been missing, and chatting through it a bit more this is not the best approach to take. Closing this PR for the time being. |
3 participants
Description
When git submodules have been deactivated, do not run any submodule related commands even if submodules are potentially detected. There's a specific choice here for them to not be used, so even for cleaning operations this should be respected.
Context
Fixes #3286
Changes
Validate that
--no-submodulesis not active when cleaning the checkout path, other locations respect this flag however this did location did not.Testing
go test ./...). Buildkite employees may check this if the pipeline has run automatically.go fmt ./...)