Add Docker Registry support #93
Conversation
* add gcp test deployment file * remove extranesou interface func * add stream error parsing
|
|
||
| authConfig, exists := getAuthConfig(remoteRepo) | ||
| // we will use GCR when deployed on GCP, so we will need to access Google Service Account token | ||
| if os.Getenv("DOCKER_PROVIDER") == "gcr" { |
There was a problem hiding this comment.
idk if I like doing it this way. Suggestions? Or is this OK?
There was a problem hiding this comment.
Yeah not ideal, you create a GetAuthConfig interface function/struct that could be injected when doing the switch on the registry provider. But it works though not sure if it's the right to be optimizing structure. Who knows...
| func NewRegistryImageCreator(repo string, reg registry.Registry) (ImageCreator, error) { | ||
| // necessary for using a GCR registry, the project name needs to be in the image tag | ||
| // and tagging is done by the LocalImageCreator before push | ||
| if proj := reg.GetProjectName(); proj != "" { |
There was a problem hiding this comment.
This is reason one for separating use of a registry into Private Docker (docker_private) and GCR (docker_gcr)...thoughts?
There was a problem hiding this comment.
Are you proposing splitting this implementation into two. One for the generic and another for GCR to avoid the if() statements? That seems neater but then we have another two implementations without much differences.
I'd say leave for now until it become a bigger problem.
pkg/kiln/docker_registry.go
Outdated
| @@ -71,8 +73,19 @@ func (imageCreator RegistryImageCreator) DeleteApplication(dockerInfo *DockerInf | |||
| name := fmt.Sprintf("%s/%s", dockerInfo.RepoName, dockerInfo.ImageName) | |||
|
|
|||
| for _, image := range *images { | |||
| // with GCR Docker Registry, you must delete the tag before the manifest | |||
| if os.Getenv("DOCKER_PROVIDER") == "gcr" { | |||
There was a problem hiding this comment.
this is reason two for separating implementation of kiln with a registry to private & GCR
Thoughts?
There was a problem hiding this comment.
LGTM there was a lot in there so I probably missed something.
For your comments: Yeah at some point in time it would be great to have some of that stuff broken out vs if statements based on ENV and also without having a lot of duplicate code. I'd leave it to you to decide if nows the right time.
| func NewRegistryImageCreator(repo string, reg registry.Registry) (ImageCreator, error) { | ||
| // necessary for using a GCR registry, the project name needs to be in the image tag | ||
| // and tagging is done by the LocalImageCreator before push | ||
| if proj := reg.GetProjectName(); proj != "" { |
There was a problem hiding this comment.
Are you proposing splitting this implementation into two. One for the generic and another for GCR to avoid the if() statements? That seems neater but then we have another two implementations without much differences.
I'd say leave for now until it become a bigger problem.
|
|
||
| authConfig, exists := getAuthConfig(remoteRepo) | ||
| // we will use GCR when deployed on GCP, so we will need to access Google Service Account token | ||
| if os.Getenv("DOCKER_PROVIDER") == "gcr" { |
There was a problem hiding this comment.
Yeah not ideal, you create a GetAuthConfig interface function/struct that could be injected when doing the switch on the registry provider. But it works though not sure if it's the right to be optimizing structure. Who knows...
kilnimplementation with Docker Registry APIregistrypkg supports GCR and Private Docker Registry backendsFixes #92
Fixes #87
Fixes #6