-
Notifications
You must be signed in to change notification settings - Fork 291
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(core): add buildargs to DockerImage #614
Conversation
Hi @black-snow, if I can offer some assistance, it looks like the number of steps just changed
I take the blame for the original test it was way too hardcoded (also notice the logs enumeration, that will be the next issue imo) : assert logs[0] == {"stream": "Step 1/2 : FROM alpine:latest"}
assert logs[3] == {"stream": f'Step 2/2 : CMD echo "{random_string} |
there are conflicts, i would like for the tests to be clarified as well, etc. |
Thanks, I'll take a look tonight. Will be an easy fix and I think I'll pull it out into a separate test. What I was thinking about: Would it make sense just to pass the kwargs down to docker-py? I'd favour keywords over kwargs every time but it's basically a wrapper around docker-py, so it might make sense to do so. Not sure about this. On the other hand - most of the args aren't likely to change. It wouldn't be too hard to duplicate and document them. |
2 notes if I may
|
@Tranquility2 ofc, feedback is always welcome!
|
Hi @black-snow just wanted to update I found a bug in the with DockerImage(path=dir, tag="test", buildargs={"MY_ARG": "some_arg"}) as image: if you still want to add this as a more direct flag I'm in favor, notice it now has a sperate test as you originally suggested 😄 |
Thanks for the heads-up and the fix @Tranquility2! I'll abandon this PR. When kwargs get passed down correctly + I get a hint in the docstring (of the constructor) I'm totally happy with not having a separate kwarg :) |
My plan is to add with DockerImage().with_build_arg(key, val) as image: Something like we have in the Container :) def with_build_arg(self, key: str, value: str) -> Self:
self._build_args[key] = value
return self Hinting is also a track I'm working on and will definitely be added as well as part of the track. see for example: #691 #692 #700 #702 |
I'm fine with a builder-style construct :) Type hinting? Excellent. I was rather relating to having "kwargs will get passed throught to the underlying docker-py" right in the docstring of |
Sounds good, I'll update. |
Adds
--build-arg
equivalent as mentioned in #610I don't get the tests to pass so this is actually untested!
Also, I'd rather move it into a separate, well-named test instead of stuffing everything in there but I didn't want to refactor so much. I'll perhaps refactor in another PR.