Fixes #14181 - Validate registry URL and attempt login #142
Conversation
| url, | ||
| credentials) | ||
| ::Docker.authenticate!(credentials, connection) | ||
| rescue => e |
There was a problem hiding this comment.
What about catching Docker::Error::AuthenticationError here?
There was a problem hiding this comment.
I can but does it deserve special treatment? I left it to catch all StandardError as to catch network problems etc...
There was a problem hiding this comment.
I guess my concern is that this could swallow exceptions that we haven't anticipated if we're catching StandardError--exceptions that have nothing to do with the actual connection.
Edit: This is fine for now though. We can change it up later if we need to.
|
Could you add a test for attempt_login? |
|
Tested and it worked. 👍 Add a test for attempt_login and I'll ACK. Thanks! |
daviddavis
added
Waiting on contributor
and removed
Needs testing
Not yet reviewed
labels
In order to avoid users trying to create containers in an external registry that doesn't exist, we should provide some preventative measures.
theforeman-bot
added
Needs testing
Needs re-review
and removed
Waiting on contributor
labels
|
@daviddavis Updated with an unit test! |
|
Shoot, I actually merged your commit (9ea4059) but it looks like master is failing. Am rerunning master and this PR and will investigate any breakages. Sorry! |
|
Green :) |
In order to avoid users trying to create containers in an external
registry that doesn't exist, we should provide some preventative
measures.