-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
feat: also check DOCKER_AUTH_CONFIG for registry auth config as an alternative to config.json #6238
feat: also check DOCKER_AUTH_CONFIG for registry auth config as an alternative to config.json #6238
Conversation
…ternative to config.json
Can you run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! just left a small suggestion. Thanks @roseo1 !
core/src/main/java/org/testcontainers/utility/RegistryAuthLocator.java
Outdated
Show resolved
Hide resolved
QQ: this is very Gitlab specific, right? I couldn't find anything official in Docker docs |
Hi @eddumelendez, sorry for the delay and thank you for the tidy up, found another way round this so forgot to check. Yes, it is very gitlab specific, not sure if that changes anything with regards to the content of the PR? |
Thanks for answering! Nothing change. I've just updated the PR with a minor change and since it will break the behavior we are holding it for a bit but indeed planning to merge it. |
@@ -16,7 +20,7 @@ | |||
public class RegistryAuthLocatorTest { | |||
|
|||
@Test | |||
public void lookupAuthConfigWithoutCredentials() throws URISyntaxException { | |||
public void lookupAuthConfigWithoutCredentials() throws URISyntaxException, IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why was IOException
added to all the throws clauses of the tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (configFile.exists()) { | ||
log.debug("RegistryAuthLocator reading from configFile: {}", configFile); | ||
return OBJECT_MAPPER.readTree(configFile); | ||
} else if (configEnv != null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the env var be evaluated first? Usually, you can use env vars to override config files but this would not be the case here (if you have both the conf file and the env var). Maybe I'm missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch! yes, you're right.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@roseo1 can you take care of this, please? If not, LMK so I can do it before merge it.
Thanks for your contribution, @roseo1 ! This has been merged in |
Resolves #3782.
Currently testcontainers-java checks docker config json for registry auth credentials. Like testcontainers-dotnet (PR testcontainers#540), would be good to also support the use of the
DOCKER_AUTH_CONFIG
environment variable.As added to the docs, would respect the current preference:
DOCKER_CONFIG
or{HOME}/.docker/config.json
DOCKER_AUTH_CONFIG
Potential impact:
Failure when attempting to lookup auth config. Please ignore if you don't have images in an authenticated registry. Details: (dockerImageName: <...>, configFile: /root/.docker/config.json. Falling back to docker-java default behaviour. Exception message: /root/.docker/config.json (No such file or directory)
Failure when attempting to lookup auth config. Please ignore if you don't have images in an authenticated registry. Details: (dockerImageName: <...>, configFile: /root/.docker/config.json, configEnv: DOCKER_AUTH_CONFIG). Falling back to docker-java default behaviour. Exception message: No config supplied. Checked in order: /root/.docker/config.json (file not found), DOCKER_AUTH_CONFIG (not set)