Open black-snow opened 1 month ago
Hi @black-snow, if I can offer some assistance, it looks like the number of steps just changed
probaly from adding
ARG {build_arg_name}
ENV {build_arg_name}=${build_arg_name}
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
What's wrong with the current test? The logic can be improved I assume, but parametrize
makes this very slim and easy to extend. Or another way to think about this, what is the expected improvement on splitting into a separate test (each separate test we add is another place that will eventually needs to be maintained when updating the core functionality)
regarding "keywords over kwargs" I tend to agree but I think we should consider that it will bloat the headers and make testing and maintenance more complex. I usually like to ask ask myself does this capability has a clear/known user - in this case do you think you will use all of them? some of them? do you know of someone that will? if the answer is yes lets add the specific stuff you are going to use. Notice both DockerClient
& DockerContainer
emphasize on giving the minimum needed + needs discovered and anything other is left to kwargs
, I find it very elegant as it keeps the code clean and feature oriented. we should try and avoid just duplicating if we don't have anything to add/change or the use-case is common (as good example to what's common is #615) this kind of additions can and will lead the more maintenance (and later on it will be impossible to remove or clean).
@Tranquility2 ofc, feedback is always welcome!
Adds
--build-arg
equivalent as mentioned in https://github.com/testcontainers/testcontainers-python/issues/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.