-
Notifications
You must be signed in to change notification settings - Fork 39
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
Fix issues pushing images to Amazon ECR #279
Conversation
Awesome, thanks! I'm working on something else today and probably can't review immediately but:
IIRC that was not a principled number but something that "seemed to work in practice", so I have no strong objection to changing it always . . . unless that causes problems for some other registry.
Yes, that type of error handling + reporting is something that needs more work in this project in general. |
This is amazing, thank you for your time and effort here @normj! |
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.
There normj:support-amazon-ecr if there is a problem that is a bit more complicated then you need to consider it not being able to process
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.
Thanks for this @normj - I'm going to merge now, and a new prerelease version of the package should be available on Github Packages shortly afterwards.
I'll update my test harness at baronfel/sdk-container-demo and check that the AWS scenario lights up (and that other scenarios aren't impacted).
Great to see this merged into. Feel free to reach out to me if there are other issues with ECR. |
I screwed up the byte calculation, for one thing. Going to patch that in momentarily. |
Looking great @normj: https://github.com/baronfel/sdk-container-demo/actions/runs/3886025610/jobs/6630637915, hard to mess with 36s from zero-to-published :) |
@baronfel Awesome! |
Note: I'm on the AWS .NET SDK team not the ECR team. I did some group debugging with the ECR team to understand the failures with this tooling pushing to ECR.
This PR address the idiosyncrasy when pushing images to ECR.
I also found that parallelizing uploading all of the layers at the same time especially with the 5 MB buffer size caused to many socket retries and ultimately failures. I changed the code for ECR to upload the layers serially. Probably more should be done there in general to better handle throttling and error reporting since currently a messy AggregateException is thrown up the chain but wanted to get feedback before doing anything more in that space.