-
Notifications
You must be signed in to change notification settings - Fork 58
feat!(git-clone): change path
input to base_dir
and return repo_dir
as output
#132
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
Conversation
After #124, we now automatically append the `path` with the `repo` name, so the example should be changed to pass only the base directory as the `path` input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fix makes sense.
I wonder if dest
/ destination
would be a more suitable name for the property now, though? It can be read as "we do a git clone and place it in destination" (same as if you were in dest
when running git clone
manually). I'm not sure what path
conveys to me in this context, is it the place we clone, is it the end name of the cloned repo, etc.
I think users would also appreciate being able to override the folder name that's created.
If we draw parallels to how git
is used:
# Both create `modules` in pwd.
git clone https://github.com/coder/modules
git clone https://github.com/coder/modules.git
# Creates `bar` in `foo` in `pwd`.
git clone https://github.com/coder/modules.git foo/bar
Git calls the this extra parameter directory
that renames the folder. I think we could get away with calling it name
, too.
Just some thoughts.
@mafredri Yes, I agree, but that would be a breaking change. We do have versioning now, and Depedanbot should handle version bumps, too. Unfortunately, Dependabot can not suggest a change of input variable names. That is why I am avoiding renaming inputs as this is out top used module. |
path
example to not use the repo namepath
example to not use the repo name and return repo_dir
as output
@mafredri Hmm, yes. So let's change it. What about |
path
example to not use the repo name and return repo_dir
as outputpath
example to not use the repo name and return repo_dir
as output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good choice for param, LGTM!
path
example to not use the repo name and return repo_dir
as outputpath
input to base_dir
and return repo_dir
as output
After #124, we now automatically append the
path
with therepo
name, so the example should be changed to pass only the base directory as thepath
input.