-
Notifications
You must be signed in to change notification settings - Fork 697
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
Use a single Docker registry #2901
Conversation
cded52a
to
3dcf58f
Compare
3dcf58f
to
b095a2b
Compare
1a75187
to
5e5ec7e
Compare
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.
Nice work, I think it looks good. The only thing I'd add is removing ACR from AKS setup (from default config, settings and aks.go).
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
We discussed with the team that there is a lack of good support for the development workflow as pointed in this discussion. We should still be able to use GCR or a different namespace from that used by the CI in the Elastic Docker registry. Latest commits address that. |
I found several weaknesses in the implemented solution. Please don't do another code review right away. |
df0b70d
to
920ea38
Compare
920ea38
to
72f4e7f
Compare
I think this is ready for a final review. Since the last review, there is no fundamental change:
One weakness that already exists and that remains is that the content of the |
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.
I think overall it's a great improvement.
I did a few tests locally and I left a few comments in the PR.
I'd be OK with merging and fixing things if they break in CI later.
For the CI, we use 3 different Docker images: for the operator, the E2E tests and the CI build tools. So far we have tried to use the Docker registry provided by the cloud provider which we are testing as much as possible. With expanding the testing to more providers we have to handle the specifities of each one especially for authentication.
This commit simplifies this by using a single Docker registry for all of our uses.
Resolves #2622.