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

Poetry is not using Dulwich to read the global git config (particularly the insteadOf option) #5934

Closed
3 tasks done
fredrikaverpil opened this issue Jun 30, 2022 · 12 comments · Fixed by #5935
Closed
3 tasks done
Labels
kind/bug Something isn't working as expected

Comments

@fredrikaverpil
Copy link
Contributor

fredrikaverpil commented Jun 30, 2022

  • I am on the latest Poetry version (master branch)
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option).
  • OS version and name: GitHub Actions
  • Poetry version: master branch

Issue

⚠️ please note that this functionality works fine with Poetry 1.1.13 but is now unsupported in the pre-1.2 master branch (unless configuring poetry@master to use the system's git client).

It doesn't seem like poetry is using Dulwich to read the global git config. Should it not do this?
I'm using Poetry in GitHub Actions with private repos and would like to rely on rewriting URLs with insteadOf:

git config --global --replace-all url.https://github.com/.insteadOf ssh://git@github.com/
git config --global --add url.https://github.com/.insteadOf git@github.com:

Dulwich recently added support for rewriting URLs 🎉 (PR here) this - but this is not picked up by poetry.

I initially raised this with the Dulwich maintainers in jelmer/dulwich#983 but they concluded the issue seems to reside with Poetry.

What is the intent here...?

  • Should Poetry use Dulwich to read the global (and the repo's?) git config?
  • What is the way forward to make Poetry read the global git config, and the insteadOf option in particular?

Cc @neersighted (as we chatted briefly about this in discord)

@fredrikaverpil fredrikaverpil added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Jun 30, 2022
@fredrikaverpil fredrikaverpil changed the title Poetry is not using Dulwich to read the git config (particularly the insteadOf) Poetry is not using Dulwich to read the global git config (particularly the insteadOf) Jun 30, 2022
@fredrikaverpil fredrikaverpil changed the title Poetry is not using Dulwich to read the global git config (particularly the insteadOf) Poetry is not using Dulwich to read the global git config (particularly the insteadOf option) Jun 30, 2022
@jelmer
Copy link
Contributor

jelmer commented Jun 30, 2022

To mimick the behaviour of C Git, poetry probably wants to call Repo.get_config_stack(), which would read both $REPO/.git/config and ~/.gitconfig

@dimbleby
Copy link
Contributor

It's not clear to me whether it should be poetry's responsibility or dulwich's responsibility to find and use your git configuration.

If poetry is the right place then I think the change you want is something like this:

-        client, path = get_transport_and_path(url, **kwargs)
+        config = StackedConfig.default()
+        client, path = get_transport_and_path(url, config=config, **kwargs)

(plus adding the import).

But I could believe that dulwich ought to have defaulted to doing that if no config is passed in to it - dulwich maintainers may have a view.

@dimbleby
Copy link
Contributor

Oh I see we have crossing updates and I imagine @jelmer is more likely to be right!

@jelmer
Copy link
Contributor

jelmer commented Jun 30, 2022

get_transport_and_path() currently defaults to not consulting any config if none is specified; we could default to consulting StackedConfig.default() instead, but I suspect that might lead to confusion since we'd be ignoring the repo-specific config.

Open to be persuaded to change this :)

@dimbleby
Copy link
Contributor

dimbleby commented Jun 30, 2022

Your implicit proposed fix seems reasonable to me, @fredrikaverpil perhaps you'd like to test and if all is well put together an MR

-        client, path = get_transport_and_path(url, **kwargs)
+        config = local.get_config_stack()
+        client, path = get_transport_and_path(url, config=config, **kwargs)

@fredrikaverpil
Copy link
Contributor Author

@dimbleby I feel like I'm not entirely sure what I'm doing as I haven't explored both codebases (poetrys, dulwichs) enough - but I would be happy to give that a try - absolutely.

@fredrikaverpil
Copy link
Contributor Author

fredrikaverpil commented Jun 30, 2022

@dimbleby @jelmer @neersighted #5935 works for my use case. 👍

This resulted in two failing tests. Looks like they all need to be treated the same way though, asserting that this new config object is part of the function call.

This is the GHA workflow I used in my private org/repos to verify this with, which runs poetry install, successfully picking up on the GHA global git config:

name: poetry

on:
  workflow_dispatch:

env:
  PYTHON_VERSION: "3.10"

jobs:
  update:
    runs-on: ubuntu-latest
    steps:

      # REDACTED - custom checkout action

      # REDACTED - custom git config setup

      - uses: actions/setup-python@v3
        with:
          python-version: ${{ env.PYTHON_VERSION }}

      - uses: fredrikaverpil/setup-pipx@v1.5

      # https://github.com/python-poetry/poetry/pull/5935
      - run: pipx install git+https://github.com/python-poetry/poetry.git@refs/pull/5935/head

      - run: pipx runpip poetry install -U git+https://github.com/jelmer/dulwich.git@master

      - run: poetry install

@fredrikaverpil
Copy link
Contributor Author

@jelmer @dimbleby I just saw you published the 0.20.44 release 🎉
Shall we include this bump in #5935 or will you open a separate PR for this?

@jelmer
Copy link
Contributor

jelmer commented Jun 30, 2022

@jelmer @dimbleby I just saw you published the 0.20.44 release tada Shall we include this bump in #5935 or will you open a separate PR for this?

If your goal with this PR is to add support for insteadOf / pushInsteadOf, you should probably bump the dulwich version in this PR.

@geoffgscott
Copy link

Poetry 1.2.0rc2 does not appear to be respecting the global git config (at least with regards to insteadOf).

@fredrikaverpil
Copy link
Contributor Author

@geoffgscott you might have to open a new issue and provide more details. It works for me, and I'm on 1.2.0 now.

Copy link

github-actions bot commented Mar 1, 2024

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants