-
Notifications
You must be signed in to change notification settings - Fork 100
Migrate image release destination registry to Azure Container Registry #699
Conversation
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
.circleci/config.yml
Outdated
@@ -154,6 +154,7 @@ jobs: | |||
publish-rc-images: | |||
<<: *base-docker-job | |||
environment: | |||
DOCKER_REGISTRY: osbapublicacr.azurecr.io/ | |||
DOCKER_REPO: microsoft/ |
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.
With this set, the final URL will be osbapublicacr.azurecr.io/microsoft/azure-service-broker:<tag>
, which looks awkward.
I think you probably want to unset this env var so that the final URL for the image will be osbapublicacr.azurecr.io/azure-service-broker:<tag>
fwiw... this env var should remain settable, just unset. The reason for this is it allows for overriding the final image URL if people want to build customized versions from their own forks and publish to somewhere else.
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.
Hmm.. I still want to keep "microsoft". With the change as you suggested "DOCKER_REPO" -> "DOCKER_REGISTRY_NAMESPACE", it looks good to me. How do you feel?
@@ -101,7 +101,7 @@ Broker chart and their default values. | |||
| Parameter | Description | Default | | |||
| --------- | ----------- | ------- | | |||
| `logLevel` | Log level (options: PANIC, FATAL, ERROR, WARN, INFO, DEBUG). | `"INFO"` | | |||
| `image.repository` | Docker image location, _without_ the tag. | `"microsoft/azure-service-broker"` | | |||
| `image.repository` | Docker image location, _without_ the tag. | `"osbapublicacr.azurecr.io/microsoft/azure-service-broker"` | |
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 (your choice) you do anything about this comment, this address also needs to change.
Overall, this looks really sensible. I made a few small comments, but overall 👍 |
This looks great! |
As the write access to microsoft org in DockerHub was revoked, we are migrating to a public ACR instance.
This PR should hold on until ACR team makes our new ACR public.