Skip to content
This repository has been archived by the owner on Feb 7, 2024. It is now read-only.

Support using commits for GClone spec searcher, correctly print errors #38

Merged
merged 3 commits into from
Jul 20, 2022

Conversation

beechtom
Copy link
Contributor

Add stderr to error message in GClone spec searcher

This fixes a bug in the GClone spec searcher that was adding the
contents of stdout to the error message raised when cloning the git
repository fails. Now, the contents of stderr are added to the error
message.

Support using commits for GClone spec searcher

This fixes a bug with the GClone spec searcher that caused git clone
to fail when provided a commit to clone. Because git clone does
not support cloning a specific commit (using a SHA1), it's necessary to
clone the entire repository. This updates the git clone command used
by the spec searcher accordingly.

This fixes a bug in the `GClone` spec searcher that was adding the
contents of stdout to the error message raised when cloning the git
repository fails. Now, the contents of stderr are added to the error
message.
@beechtom
Copy link
Contributor Author

@glennsarti Unfortunately this makes the GClone spec searcher a bit slower, but as far as I can tell you can't clone a bare repository using a specific commit. :/

@beechtom
Copy link
Contributor Author

beechtom commented Jul 7, 2022

@glennsarti can I get a review on this when you have a chance?

@beechtom
Copy link
Contributor Author

@glennsarti Anything I can do to help get this over the line?

@glennsarti
Copy link
Owner

Apologies @beechtom Work and Life getting in the way..I'll poke at this soon...

Anything I can do to help get this over the line?

@logicminds was looking at getting this moved to Vox Pupuli which would help :-) So I'm not the single maintainer

@logicminds
Copy link
Contributor

logicminds commented Jul 19, 2022

vox was not keen on taking on additional debt so this has not occurred yet. Cloning the entire repository will likely lead to drastically longer load times for things like stdlib, ntp and anything that has large files.

Can you add a test for ref/commit. Seems as though my test ref is actually a tag would doesn't exercise the bug. Please add a test to illustrate the bug and test your code.

Removing --single-branch is also not a good idea IIRC

This fixes a bug with the `GClone` spec searcher that caused `git clone`
to fail when provided a commit to clone. Because `git clone` does
not support cloning a specific commit (using a SHA1), it's necessary to
clone the entire repository. This updates the `git clone` command used
by the spec searcher accordingly.
@beechtom beechtom force-pushed the bug/gclone branch 2 times, most recently from ad0ca95 to 282e0e1 Compare July 19, 2022 20:41
@beechtom
Copy link
Contributor Author

beechtom commented Jul 19, 2022

@glennsarti My team has also said we'd be up for forking / helping out with maintainer responsibilities if you are open to that (and if Vox is not interested in taking it on).

@logicminds Added a test for using a commit. I'm aware that this change can lead to longer load times. The spec searcher needs to support using commits/SHAs as refs, as Puppetfile supports them, and cloning a bare repository using a commit/SHA does not work.

I've added some logic that will attempt to clone a bare repository first, and then fall back to cloning the full repository if that fails. That should speed things up when the ref is a tag or branch.

(Also, --single-branch does not seem to have an affect when cloning a full repository and is implied when using --depth=1.)

@logicminds
Copy link
Contributor

Can you add a test for a bad ref as well. Otherwise this is looking like a good work around.

@glennsarti
Copy link
Owner

My team has also said we'd be up for forking / helping out with maintainer responsibilities if you are open to that (and if Vox is not interested in taking it on).

@beechtom Yeah, if you want to contact me on Twitter or via email we can discuss it. forking etc. is easy, transferring ownership of the gem is the only sticking point.

This updates the `GClone` spec searcher to start by attempting to clone
a bare repository. If cloning a bare repository fails, the spec searcher
falls back to attempting to clone the full repository. Because
Puppetfile supports using commits/SHA1s as refs when installing git
modules, puppetfile-resolver needs to support them as well. Cloning bare
repositories with a commit/SHA1 fails, so falling back to a full clone
ensures that the resolver supports this behavior.

The spec searcher could always clone a full repository instead of
first attempting to clone a bare repository, but this might dramatically
increase the time it takes to clone a module.
@beechtom
Copy link
Contributor Author

@logicminds Added a failing case.

@logicminds
Copy link
Contributor

👍 looks good to me.

@glennsarti glennsarti merged commit fe3a850 into glennsarti:main Jul 20, 2022
@glennsarti
Copy link
Owner

will do a release soon

@glennsarti
Copy link
Owner

Released as 0.6.2

@beechtom
Copy link
Contributor Author

@glennsarti Thank you!

@beechtom beechtom deleted the bug/gclone branch July 21, 2022 17:43
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.

3 participants