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

            assert logs[0] == {"stream": "Step 1/2 : FROM alpine:latest"}
            assert logs[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.