Incorrect username/password gives misleading error

[patter001]

Pull request to go with issue 4833

Closes #4833

[xavfernandez]

I'm not sure we want an InstallationError on this one (meaning the pip install command will fail), a simple logger.warning might maybe be enough ?

[xavfernandez]

It would help the user to know the affected url.

[pradyunsg]

@xavfernandez requested changes in a comment.

[patter001]

I will add the URL. I believe it needs to be error/exception or it will still result in an installation error (as it does now) and you will just have two messages for the one error. I would be happy to change the exact error though. Should I make a new one Error? or would you prefer PipError or ConfigurationError?

[patter001]

Unfortunately, I don't understand the pypy3 error...It seems to be some time out unrelated to my update that prints the URL.

[pradyunsg]

@patter001 You'll want to rebase this/merge master since there's been some updates to the CI stuff since you opened this PR.

None the less, I've restarted the failed CI job, it's mostly a transient error. :)

[patter001]

@pradyunsg Thanks! I see it is passing now, so let me know if there are more changes requested or if this is enough for the merge (the re-base would only be needed if I needed to re-submit more changes right?)

[pradyunsg]

@xavfernandez's concern about InstallationError seems to not have been addressed yet.

---

For the news fragment, could you add something relevant for the news file -- like pip now informs user about 401 errors (except worded better)?

I'd really like it if you could add a test otherwise would go away in the next refactor. ;)

---

the re-base would only be needed if I needed to re-submit more changes right?

If you could merge master into this PR, it would be ideal. There have been some changes in the CI configuration so it would be needed that the tests with that updated configuration.

[patter001]

I need an answer to my question to better address @xavfernandez's concern...or I'll be happy to just make a new error condition.

There are no existing tests here around a fake host and prompting the username/password to leverage from.

[pradyunsg]

I need an answer to my question to better address @xavfernandez's concern...or I'll be happy to just make a new error condition.

Oh, I missed that. I'd wait for his response then. :P

There are no existing tests here around a fake host and prompting the username/password to leverage from.

Cool. No issues then.

---

That makes my remaining concern to just the news fragment. I'll defer to @xavfernandez for the functional parts of this PR.

[patter001]

Just figured I'd check in: @xavfernandez's do you agree we need InstallationError now? I did try a different error, but those result in tracebacks instead of the nice clean exit...so I'm open to suggestions.

I did the rebase and the news update...still got the random timeout failure. I'm not sure how to re-run that without actually pushing a new change.

[xavfernandez]

Well if you use several indexes, having a 401 on a single index does not automatically mean you won't be able to perform your install so why completely abort with an InstallationError ?

And if you only have one index, you'll get the cannot find distribution with a red logger.warning indicating the 401 issue.

[patter001]

The “cannot find distribution” is too vague and the user cant tell between the 401 or mistyping the package name. Would you rather just have an additional print with the url and then let it go on to hit the existing error? I guess I don’t care as long as the user gets a better indication of went wrong.

…

On Fri, Nov 17, 2017 at 4:48 PM Xavier Fernandez ***@***.***> wrote: Well if you use several indexes, having a 401 on a single index does not automatically mean you won't be able to perform your install so why completely abort with an InstallationError ? And if you only have one index, you'll get the cannot find distribution with a red logger.warning indicating the 401 issue. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4834 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJfOxH_M4DUrpeBUH7EuPgNBl4q3Z3slks5s3g0-gaJpZM4QRmOc> .

[patter001]

I have made the modifications to just print the warning.

I did the rebase, but cannot get the python3.3 part to pass (and seem to have no bearing on my changes). Please provide any feedback for whether the patch is sufficient to get merged or not.

[xavfernandez]

Rebasing on master should fix the 3.3 error.
The empty lines changes are not needed though.

[patter001]

fixed

[pradyunsg]

These newline changes seem unrelated... Could you remove them or is there some reason I'm not seeing?

[lock]

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