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 clone recursive submodules option #1488

Merged

Conversation

charlysotelo
Copy link
Contributor

@charlysotelo charlysotelo commented Dec 18, 2024

First of all -- amazing tool folks. Great work.

My use case involves using git submodules to reuse a .devcontainer directory across many git projects. Devpod currently fails as before this PR there is no way to do this. This PR aims to add a git-clone-recursive-submodules option to remedy this.

To this aim, I've refactored git/clone.go with the options pattern, which should make any future git option updates trivial, and implemented the passing of this option as one of them.

I set up a little test repo with the scenario:

【=◈︿◈=】@ C02FK0TXML7L ➜  tree -a
.
├── .devcontainer # is a submodule
│   ├── .git
│   └── devcontainer.json
└── main.go

https://github.com/charlysotelo/devcontainer-submodule-test

Here you can see the tool currently failing to find my submoduled .devcontainer/devcontainer.json (and falling back to using a go devcontainer):

【=◈︿◈=】@ C02FK0TXML7L ➜  devpod up --provider docker github.com/charlysotelo/devcontainer-submodule-test --ide none
21:40:33 info Creating devcontainer...
21:40:34 info Clone repository
21:40:34 info URL: https://github.com/charlysotelo/devcontainer-submodule-test
21:40:36 done Successfully cloned repository
21:40:36 info Couldn't find a devcontainer.json
21:40:36 info Try detecting project programming language...
21:40:36 info Detected project language 'Go'
21:40:36 info Inspecting image mcr.microsoft.com/devcontainers/go
21:40:36 info 9b68369cd7af341ac66cda034cbdd490a063632a0b3186c3ca9fae48c99067bb
21:40:39 info Setup container...
21:40:40 info Chown workspace...
21:40:40 info Run 'ssh devcontainer-submodule-test.devpod' to ssh into the devcontainer

Here it is shown working with this PR:

【=◈︿◈=】@ C02FK0TXML7L ➜  git:(feature/git-recursive-submodules) ./devpod-cli up --provider docker github.com/charlysotelo/devcontainer-submodule-test --ide none --git-clone-recursive-submodules
21:33:57 info Creating devcontainer...
21:33:57 info Clone repository
21:33:57 info URL: https://github.com/charlysotelo/devcontainer-submodule-test
21:34:02 done Successfully cloned repository
21:34:03 info Inspecting image mcr.microsoft.com/devcontainers/python:3
21:34:03 info Image mcr.microsoft.com/devcontainers/python:3 not found
21:34:03 info Pulling image mcr.microsoft.com/devcontainers/python:3
21:35:07 info cdaacf302617061a6d4fe2a41aca944e5340865c476f104663c60ae990745a47
21:35:16 info Setup container...
21:35:17 info Chown workspace...
21:35:17 info Run 'ssh devcontainer-submodule-test.devpod' to ssh into the devcontainer

While I'm not new to golang, this is my first time contributing to an open-source project, so your patience and feedback on the PR is appreciated.

Cheers.

@charlysotelo charlysotelo marked this pull request as draft December 18, 2024 03:03
@charlysotelo charlysotelo force-pushed the feature/git-recursive-submodules branch 2 times, most recently from 47e43ba to ffb9327 Compare December 18, 2024 03:25
@charlysotelo charlysotelo marked this pull request as ready for review December 18, 2024 03:25
@pascalbreuninger
Copy link
Member

@charlysotelo Thanks for your contribution, I'll take a look at it. I'm not the biggest fan of submodules in general but this seems to be a pretty interesting use case

Copy link
Member

@pascalbreuninger pascalbreuninger left a comment

Choose a reason for hiding this comment

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

LGTM overall 🙌
I'm curious if we could enable this by default but to answer this we'll have to do some digging into git clone performance with the --recurse-submodules flag set

The lint pipeline failure should be resolved if you rebase your branch onto the latest main

@charlysotelo charlysotelo force-pushed the feature/git-recursive-submodules branch from ffb9327 to 73d7c95 Compare December 18, 2024 13:51
@charlysotelo
Copy link
Contributor Author

charlysotelo commented Dec 18, 2024

Thank you!

I've gone ahead and rebased.

As for enabling it by default, I'd suggest punting on that question and if enabling it by default is desired, just do so on a new minor semantic version.

@pascalbreuninger pascalbreuninger merged commit 0c82b3f into loft-sh:main Dec 19, 2024
15 of 23 checks passed
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.

2 participants