Avoid hidden interactive ssh prompts at low verbosity levels.

[Yarn]

Pull Request Check List

Resolves: #9618

• Added tests for changed code.
• Updated documentation for changed code.

[dimbleby]

I don't like having this depend on verbosity, the different behaviour is bound to be confusing one day. Eg imagine someone trying to debug their CI pipeline by turning up verbosity.

Does the error message make sense when this fails? I'd think we might need to catch an exception somewhere and make sure the log says which repository / host it is that we are failing to clone.

[Yarn]

i agree having it tied to verbosity isn't ideal but I think having an escape hatch to access interactive git prompts is practically helpful

perhaps a better solution would be to disable the output of the Resolving dependencies... (3.6s) line while git operations are running so any interactive prompts can display correctly

---

i'm looking at the dulwich source and it's actually more prone to cause unexpected behavior than it initially seems. it appears that passing ssh_command as a kwarg makes dulwich ignore the GIT_SSH_COMMAND and GIT_SSH environment variables so it could easily change other ssh behavior unintentionally based on verbosity settings

looking into the CI failures it seems that dulwich just blindly passes kwargs to whatever underlying class it decides to use based on the url so it fails with non ssh urls...

[dimbleby]

The only behaviour that should change when a user configures verbosity, is verbosity.

Not sure I follow dulwich issue, but I guess report it there? These things are never so straightforward as you'd hope, are they?

[Yarn]

so i've confirmed stopping the console progress indicator gets the output working, currently i just hacked it together with a global. I'm working on something a bit more robust

---

it looks like the dulwich was fixed in upstream for this specific case (after the most recent pypi release) https://github.com/jelmer/dulwich/blob/06405c8e29e0d3716384e582c1b83a8e05fa3563/dulwich/client.py#L699

before, passing ssh_command in kwargs would fail if it wasn't a git+ssh url since it would eventually get passed to GitClient which didn't accept that argument. it looks like the issue still exists for the file scheme
https://github.com/jelmer/dulwich/blob/06405c8e29e0d3716384e582c1b83a8e05fa3563/dulwich/client.py#L2811

it looks like it's been reported previously
jelmer/dulwich#1339

[dimbleby]

not sure what you have in mind with progress indicator but if it is even slightly complicated or controversial I would instead continue in the direction this pull request was going

• unconditionally set ssh_command for dulwich
• wait for (or ask for) a dulwich release allowing this to work
• per earlier comment, consider whether the poetry logging is then good enough for users to understand what is happening: and if not then do something about it

[Yarn]

the new solution I just pushed has the downside that the timer will pause during git operations even if there is no interactive input but I think fixing that would be significantly more complicated. beyond causing the timer to pause when it maybe shouldn't I think this change should be pretty safe

demo with time.sleep
the error is just because I commented out the actual git command
shot_20240817_130913_.webm

---

Without the interactive prompt the error is just "Host key verification failed.", verbose or not git errors don't seem to indicate what dependency they failed on.

other git errors such as authentication failures are largely the same, e.g. just "[email protected]: Permission denied (publickey)."

---

I think breaking the GIT_SSH_COMMAND environment variable should absolutely be avoided. It covers a lot of use cases that poetry presumably doesn't want to try to create bespoke solutions for. I frequently use it to specify which private key should be used but it covers many more esoteric cases as well.

[dimbleby]

I still prefer to head in the direction of never waiting for input: eg for CI use, and also for consistency - cf #5880 (when poetry uses the system git). But possibly others will feel differently

it should be easy for poetry to add a better log, just add broader exception handling here that logs the url and re-raises.

Not sure about GIT_SSH_COMMAND, in some ways I would prefer poetry just to have code that works and not be exposed to users screwing it up. But one could take the view that if someone has set that environment variable then we can assume they know what they are doing and respect that: else ssh -o BatchMode=yes.

[Yarn]

how about

• by default ssh -o BatchMode=yes is used. GIT_SSH_COMMAND/GIT_SSH enviornment variables override this
• any git related errors have a message telling the user to set GIT_SSH_COMMAND=git if they expected an interactive prompt
• if GIT_SSH_COMMAND is set always print a warning reminding the user that -o BatchMode=yes is set by default

[dimbleby]

1. sounds fine.

2. I think you meant "ssh" not "git". But I still think that trying to do interactive stuff within the poetry context is the wrong direction. I'd just report the error and let readers figure it out - most errors are likely nothing to do with this - or perhaps advise them to verify that they can git clone directly.

3. sounds excessive and will annoy the power users who set the variable. A debug log sounds like a good idea though, so folk have a chance of figuring it out when they mess up.

Progress in this direction is blocked on a dulwich release, though.

[Yarn]

2. yes, GIT_SSH_COMMAND=ssh. i've been on the receiving end of "and let readers figure it out" enough times that i'm pretty strongly opinionated on wanting a bit of extra noise in the output on errors over needing to go hunting through source code to figure out how a tool is invoking git/ssh.

notably in this case, with the proposed changes, cloning the repo manually can work while poetry doesn't. the extra output is specifically to try to make sure the user can get from working git clone to working poetry, ideally without making them read the poetry source code

I think between interactive authentication and ssh key passphrases just telling people to simply not ™️ isn't reasonable, even if we prioritize CI/CD behavior by default

3. having it be debug (or info so it shows with -v and -vv, not just -vvv) seems ok. i'm just imagining a case where someone adds GIT_SSH_COMMAND="ssh -i /key" to a CI/CD workflow and their error turns into the job hanging forever

4. we could just set GIT_SSH_COMMAND with a context manager and bypass any issues with the dulwich kwargs. this should also work the same for dulwich and the system git client

[dimbleby]

You want to write a log aimed at exactly this issue because that is what you have hit and are now thinking about. But unless we have a highly specific exception telling us what has gone wrong, a highly specific error message is likely to be counterproductive for all the other possible errors.

That is why I suggest just reporting whatever error we get, only adding a little more detail eg exactly which url it was that poetry could not clone.