-
Notifications
You must be signed in to change notification settings - Fork 273
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
feat: add pull command #1681
feat: add pull command #1681
Conversation
What we've seen work is that with AWS you can set the cred helper on your local dockerconfig. It does mean extra local configuration step, so perhaps opting into using cluster credentials would be preferable? |
5464891
to
636327b
Compare
97b3348
to
a01322f
Compare
a01322f
to
5bfab26
Compare
Sorry @swist I missed your comment there a while back. I'm not sure we can use cluster credentials in a portable fashion. We should however definitely document the extra step for ECR auth. |
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.
Overall looks like a solid approach, just a few comments. Also, an integ test would be great. Let me know if I can help with that.
f9ca573
to
a7f7cc5
Compare
60096d5
to
823a596
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.
Structure and code LGTM 👍 just some super minor comments + a couple of thoughts on the tests.
garden-service/test/integ/src/plugins/kubernetes/commands/pull-image.ts
Outdated
Show resolved
Hide resolved
2124531
to
aba71f9
Compare
aba71f9
to
7bc460d
Compare
What this PR does / why we need it:
Similarly to the
garden publish
command, this adds agarden pull
command that works for the kubernetes/container module types.A use case: a frontend engineer wants to run a local copy of an image but doesn't want to build the image (building takes longer than pulling down an image).
Which issue(s) this PR fixes:
Fixes # #1647
Special notes for your reviewer:
This doesn't work at the moment for external registries where auth is required that isn't currently supported (i.e. ecr with
credHelpers
). We could perform a 2 step dance, however:I'm all open to other ideas that are perhaps simpler though.
@edvald @eysi09