use Pull through cache #763
Conversation
resources/azure/environment.go
Outdated
| @@ -64,6 +64,10 @@ func (e *Environment) InternalRegistry() string { | |||
| return "none" | |||
| } | |||
|
|
|||
| func (e *Environment) InternalDockerhubMirror() string { | |||
| return "none" | |||
There was a problem hiding this comment.
You can return public DockerHub so that it does not fail: registry-1.docker.io
|
/merge |
|
🚂 MergeQueue This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Use |
dd-devflow
bot
added
mergequeue-status: removed
and removed
mergequeue-status: waiting
labels
|
This merge request was unqueued If you need support, contact us on Slack #devflow! |
| @@ -39,9 +45,20 @@ func Run(ctx *pulumi.Context) error { | |||
| return err | |||
| } | |||
|
|
|||
| // Install docker if not installed yet, we need it to configure docker credentials | |||
| _, dockerInstallCmd, err := docker.NewManager(*awsEnv.CommonEnvironment, vm) | |||
There was a problem hiding this comment.
We need docker installed before running ConfigureECRCredentials otherwise docker login fails.
This is not ideal because the NewKindCluster already installs docker but it works because docker installation is idempotent.
We could do the installation of docker oustide of NewKindCluster or add a hook to configure the credentials after docker is installed. The latter option would be easier if all the Environment to implement a common interface (#688)
There was a problem hiding this comment.
It's fine, I am wondering if we should have it inside the NewKindCluster component, as it needs it. Can be done later
There was a problem hiding this comment.
Would have been easier, but we do not want to put AWS specific logic in the NewKindCluster component. That could be used on any cloud provider
There was a problem hiding this comment.
We'll need to duplicate this code in datadog-agent with current code. However you could have a WithECRCredentials that implements this. Note the ExtraMount you're adding means that it will currently fail when not using any credential helper. You may need to always create a empty {} JSON file.
You also don't need docker login, what docker login does is very basic JSON gen. The way I see it the clean way would be to build a small Go code that imports https://github.com/awslabs/amazon-ecr-credential-helper/blob/main/ecr-login/ecr.go#L83-L110 and produces the config.json on stdout.
It's much smaller and faster to download then the AWS SDK and you have the option to package as a docker container and get the config.json with docker run --network host <your_helper_image> > config-tmp.json.
| @@ -39,9 +45,20 @@ func Run(ctx *pulumi.Context) error { | |||
| return err | |||
| } | |||
|
|
|||
| // Install docker if not installed yet, we need it to configure docker credentials | |||
| _, dockerInstallCmd, err := docker.NewManager(*awsEnv.CommonEnvironment, vm) | |||
There was a problem hiding this comment.
It's fine, I am wondering if we should have it inside the NewKindCluster component, as it needs it. Can be done later
| @@ -154,3 +170,36 @@ agents: | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| func ConfigureECRCredentials(e resAws.Environment, vm *remote.Host, arch os.Architecture, opts ...pulumi.ResourceOption) (*goremote.Command, error) { | |||
There was a problem hiding this comment.
💬 suggestion
If this is meant to be used from datadog-agent, could be moved to docker or some helpers package, rather than scenarios.
There was a problem hiding this comment.
Since it is something specific to AWS I did not want to have it in components/docker that should remain cloud agnostic. But yes we can probably find a better place for it
| architecture = "aarch64" | ||
| } | ||
|
|
||
| unzipInstallCommand, err := vm.OS.PackageManager().Ensure("unzip", nil, "", opts...) |
There was a problem hiding this comment.
💭 thought
Unrelated, noticing now that calls to Ensure do not pin versions, might be something we work on
There was a problem hiding this comment.
I think replacing unzip by unzip==<version> could work. But would need to be tested
There was a problem hiding this comment.
I am not sure there is an added value in pinning versions, for two reasons:
- Versions are usually pinned by distro release (normally only minor/security changes).
- We don't pin AMIs and in most cases we don't even pin OS version.
There was a problem hiding this comment.
We do pin some AMIs, for example for the installer tests. We don't probably want to maintain all images and versions, but for folks who would like to, we might want to offer the option. No one ever asked for it, just noticing it now.
What does this PR do?
Which scenarios this will impact?
Motivation
Additional Notes