Skip to content

Use a more generic way to pull repo name#81

Open
efimk-lu wants to merge 2 commits into
jacob-meacham:developfrom
efimk-lu:develop
Open

Use a more generic way to pull repo name#81
efimk-lu wants to merge 2 commits into
jacob-meacham:developfrom
efimk-lu:develop

Conversation

@efimk-lu

@efimk-lu efimk-lu commented Sep 23, 2021

Copy link
Copy Markdown

Extracting the repo name from the folder into which the git repo was cloned is dangerous. Mainly because the folder name can change. This is a more robust way of pulling it.

See https://stackoverflow.com/a/42543006

Because folder name can change, getting the repo name from the folder name is dangerous. See https://stackoverflow.com/a/42543006
@efimk-lu

Copy link
Copy Markdown
Author

Running git config --get remote.origin.url will return for example git@github.com:lumigo-io/python_tracer.git

Comment thread src/index.js Outdated
Co-authored-by: Jay Vercellone <jverce@users.noreply.github.com>
@efimk-lu

Copy link
Copy Markdown
Author

Hey @jverce, how can I merge the PR?

Comment thread src/index.js
case 'repository': {
const pathName = await _exec('git rev-parse --show-toplevel')
value = path.basename(pathName)
const firstRemoteName = await _exec('git remote | head -1')

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I wonder if the better option here would be to default to origin and allow a config escape hatch instead of assuming the first remote is the right one (I also think this isn't guaranteed to work on all systems, since not all systems have access to head)

@efimk-lu efimk-lu Oct 10, 2021

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

@jacob-meacham thinking out loud, no need to change the default behavior, so we won't break any legacy system using this plugin, and use a config escape to override the behavior and use git config --get remote.origin.url . WDYT?

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

I'm compelled by your case - I think that this is strictly more correct (though I'd release it as a breaking change), so I think we should just go for it and either add configuration to set the remote you want to pull or just use origin which I expect is right the vast majority of cases

@jverce

jverce commented Jan 4, 2022

Copy link
Copy Markdown

Any updates on this one?

This is especially needed when running on CI/CD pipelines like CircleCI where they clone the repo onto a generically named folder called project and we end up with something like this:
image

@efimk-lu

efimk-lu commented Jan 5, 2022

Copy link
Copy Markdown
Author

Hey @jverce, not yet.

@jacob-meacham

Copy link
Copy Markdown
Owner

Apologies, this fell off my radar! @efimk-lu would you mind adding a legacy escape for people relying on the old behavior, along with config to explicitly set the remote (defaulting to the first in the list is fine, but nice to let users specify a remote). Once that's done I'll merge and release

@f00f

f00f commented Apr 26, 2022

Copy link
Copy Markdown

@jacob-meacham Using the folder name is annoying. Would this be the config logic you want? I might take the time to implement it. Can someone hint me at docs on how to define config parameters?

(parameter names below are just for illustration purposes. feel free to suggest good names)

If remote is configured, use that
Else, if use_first_remote is configured to true, use first remote
Else: old behaviour

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.

4 participants