-
-
Notifications
You must be signed in to change notification settings - Fork 546
feat: create an internal sdk for docker contexts and docker configs #3091
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
base: main
Are you sure you want to change the base?
Conversation
it contains: - docker config - docker context discovery
✅ Deploy Preview for testcontainers-go ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@stevenh I think this PR would be the foundation for the internal SDK in which the work for the new docker client could land. Can we prioritise this PR if possible? |
|
||
// getContextFromEnv returns the context name from the environment variables. | ||
func getContextFromEnv() string { | ||
if os.Getenv(EnvOverrideHost) != "" { |
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.
bug: this looks like it should be an == ""?
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.
This is the original code from docker/cli, so it must work as is
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.
Interesting as that specifically breaks context if you specify a host. This is documented in the comment for CurrentContext
. We should replicate that so folks know the history if we want to keep that behaviour as it's quite unexpected.
if ctxName := os.Getenv(EnvOverrideContext); ctxName != "" { | ||
return ctxName | ||
} | ||
|
||
return "" |
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.
suggestion: simplify to only one read so it also closes the race condition.
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.
we are reading two different variables, is that what you meant?
* main: deps(aerospike): replace core module in go.mod (testcontainers#3116)
* main: chore(ci): run core tests on Testcontainers Cloud (testcontainers#3117)
* main: feat: add toxiproxy module (testcontainers#3092)
What does this PR do?
This PR adds two packages into a new internal docker package:
The config package embedes the cpuyguy83's
dockercfg
package, to have control on the config and auth types.Why is it important?
Both new packages could represent the seed for an eventual docker-sdk-go package, that would naturally emerge from this internal set of packages.
They are internal in order to iterate faster, and not break clients of testcontainers-go.
Related issues