Skip to content
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

Closed
wants to merge 2 commits into from

Conversation

black-snow
Copy link
Contributor

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
Copy link
Contributor

Tranquility2 commented Jun 23, 2024

Hi @black-snow, if I can offer some assistance, it looks like the number of steps just changed
image
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
Copy link
Collaborator

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

@black-snow
Copy link
Contributor Author

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
Copy link
Contributor

Tranquility2 commented Jun 23, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

Tranquility2 commented Oct 1, 2024

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

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

@black-snow
Copy link
Contributor Author

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

@black-snow black-snow closed this Oct 1, 2024
@Tranquility2
Copy link
Contributor

Tranquility2 commented Oct 2, 2024

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

@black-snow
Copy link
Contributor Author

black-snow commented Oct 2, 2024

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
Copy link
Contributor

Sounds good, I'll update.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants