feat(core): add buildargs to DockerImage

[black-snow]

Adds --build-arg equivalent as mentioned in #610

I 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.

[Tranquility2]

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) :

assertlogs[0] == {"stream": "Step 1/2 : FROM alpine:latest"}
assertlogs[3] == {"stream": f'Step 2/2 : CMD echo "{random_string}

[alexanderankin]

there are conflicts, i would like for the tests to be clarified as well, etc.

[black-snow]

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.

[Tranquility2]

2 notes if I may

1. 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)

2. 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 feat(core): allow custom dockerfile path for image build and bypassing build cache #615) this kind of additions can and will lead the more maintenance (and later on it will be impossible to remove or clean).

[black-snow]

@Tranquility2 ofc, feedback is always welcome!

1. I think parameterized tests are fine if the parameters are for the same behaviour, if you want to cover a bunch of different inputs to test edge cases and perhaps even defaults (arguably). I'd split different behaviours (or features) into explicit tests. If a test breaks you know exactly what's wrong and the tests are living documentation.
2. True, my thoughts exactly. But should we then accept kwargs and pass them through to docker-py with a hint to check their docs? I don't really need build args right now, but I'd absolutely want to be able to use them when I need them.

[Tranquility2]

Hi @black-snow just wanted to update I found a bug in the buildargs implementation #706
Now fixed @ #708 so you can use:

withDockerImage(path=dir, tag="test", buildargs={"MY_ARG": "some_arg"}) asimage:

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 😄
(If you need any help just tell me)

[black-snow]

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 :)

[Tranquility2]

My plan is to add

withDockerImage().with_build_arg(key, val) asimage:

Something like we have in the Container :)

defwith_build_arg(self, key: str, value: str) ->Self:
self._build_args[key] =valuereturnself

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

[black-snow]

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 __init__ (or wherever it makes most sense). I remember that I was looking for a way to pass args but it just wasn't apparent how I could do that. I had to go into the code to see how. I'd rather either have an explicit kwarg or I hit ctrl+space, scan through the docstring and see that I can just pass through whatever :)

[Tranquility2]

Sounds good, I'll update.