-
Notifications
You must be signed in to change notification settings - Fork 52
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
Webhook support #25
Comments
Thanks for your interest & for extending dregsy! So far I've only skimmed the changes in your fork, so these are just a few initial, high level thoughts & comments: I can see three major areas in which you have made changes:
I think all of these topics are of interest for inclusion into dregsy. It would be great though if we could do that via dedicated issues + PRs, to have cohesive change sets with separated concerns. Now for the topics:
|
Thanks for having a look so fast! You're indeed very right about those 3 domains and we will of course split into 3 PRs for inclusion. We had to iterate fast and that's also why we have duplicated some code parts on purpose: to avoid breaking too much stuff or existing behavior while getting enough flexibility to add features. There will be a refacto effort before shipping code to a PR.
You will note there are 2 kind of webhook tasks: We did not made any performance test and are not able to say how it can differ or not from a sequential model since running on a AKS cluster makes a single push very fast and difficult to be compared to multiple simultaneous push. About concurrent use of registry, we did not notice issues with it and I think even writing a exact same layer by 2 different processes is an atomic operation and should not generate an error or unexpected behavior. There is also place for optimization: when we receive 3 pushes of the exact same image with 3 distinct tags at the almost same time, this will result in writing the layers 3 times. We could use a dedicated queue per manifest (using its sha256)
I understand your last point. It seems to me difficult to integrate it like another binary, mostly because configuration would have to be (re)written in a file for each hook. Forking the code seemed to us the most natural way to achieve it in the right amount time and with a pretty good reliability. We will come back to you as soon as we have news and are ready for some reviews. We very recently added retry (because we are having about 1 fail over 1000 and it can easily be reduced with a retry) and are working on bearer token for auth. Thanks! |
Regarding the approach with your current fork, that's no problem at all, I understand your situation. It's important that you get the problem at hand solved quickly. If dregsy could help you a little bit with that, great! As you gain more experience with your implementation, we can spin out the different aspects one by one, discuss, and merge upstream. So we can take this step by step, no pressure. I myself don't have that much bandwidth at the moment to invest in dregsy, it's mostly a side project for me. So taking the slow route is totally fine 😉
Likewise, thanks a lot again for taking the time to think about enhancements and implementing them! That's why I like Open Source 👍 |
Hi @xelalexv, thanks for providing this tool which has a lot of helpers for ECR compared to "raw Skopeo" (mainly update auth tokens and creating a repo if does not exist).
At Padoa, we are using it to keep in sync repositories from 2 distinct cloud provider managed registry instances: Azure Container Registry to AWS ECR.
The best way we found currently is to send a webhook at each manifest push on the origin so that it triggers a sync of the pushed tag. We hence added a support for incoming webhook as well as support for multiple parallel sync.
It is still a draft (we also need to update doc) and work is in progress but tell us if it is something you would like to include in the original dregsy, and how we can contribute in this way if so.
The fork is available here: https://github.com/padoa/dregsy/
Thanks in advance for any answer :)
The text was updated successfully, but these errors were encountered: