Closed skshetry closed 8 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Comparison is base (
5bb89fc
) 100.00% compared to head (0a25b7c
) 100.00%.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
@skshetry Why even have --no-open
? Is it enough to mention to click the link if it doesn't open?
Edit:
But backward compatibility is still maintained to avoid creating a major version release.
Is this the only reason? If so, that's fine, just checking I'm not missing something.
@skshetry Why even have
--no-open
? Is it enough to mention to click the link if it doesn't open?
I thought the same before, but I think it's worth to provide a choice. It might be useful if you are working on remote machines where you have the browser but don't want to login to directly. Eg: using RDP to access remote hosts.
But maybe what we need is dvc studio login --token=<token>
.
I don't follow. What does dvc studio login --no-open
do that's different from the default? Both seem like you need to go to the same URL to authenticate, whether it's opened automatically or not.
If that fails, should we link to the settings where you can get the token and explain how to set it manually?
I don't follow. What does
dvc studio login --no-open
do that's different from the default? Both seem like you need to go to the same URL to authenticate, whether it's opened automatically or not.If that fails, should we link to the settings where you can get the token and explain how to set it manually?
Yes, but the machine you want to login can be different if you are working on a remote machine. The default will open on the remote machine, whereas you may want to do the login flow on your machine.
I don't think this is a strong reason. But I do prefer to give an escape hatch to users in these kinds of "automation" where users might find opening browsers' automatically annoying. (Or, opens a wrong browser for some reason, etc.).
Again, I don't have a strong opinion here. I'm happy to get rid of it. Or, keep --device-code
initial implementation.
I think the message is too verbose. Also removed
use_device_code
specific logic. Now it'll show a URL with 'device_code' included, instead of requiring user to open a url separately and then copy the code. Theuse_device_code
is effectively nowopen_browser=False
.So I'm deprecating it, and introducing
open_browser=True|False
to do just that. But backward compatibility is still maintained to avoid creating a major version release.Related: https://github.com/iterative/dvc/pull/10297