fix image build not using cache #559
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: fulminemizzega The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/packit retest-failed |
There was a problem hiding this comment.
thanks @fulminemizzega for this PR. please address my comments and we'll move forward :) also don't hesitate to ask questions or raise concerns
podman/domain/images_build.py
Outdated
| def default(value, def_value): | ||
| return def_value if value is None else value |
There was a problem hiding this comment.
having a function for this is overkill, parameters can be defined as default in kwargs calls.
consider using kwargs.get(value, default) like here
There was a problem hiding this comment.
I did not know of this. Makes more sense
There was a problem hiding this comment.
This should be ok now. I'm a bit on the fence on this, I started thinking about using an enum type for the outputformat parameter, so I've played a bit with the code in another branch in my fork and removed all the kwargs thing and moved all the keys to function parameters, with type annotations and default values. Why do so many functions in this project use kwargs and a _render_param function? Is this something that could be useful or is it better if I leave it alone?
There was a problem hiding this comment.
Why do so many functions in this project use kwargs
Historical reasons probably?
@jwhonce might give us a good answer
There was a problem hiding this comment.
@fulminemizzega kwargs was a good escape hatch for podman-py to ease the migration for scripts from docker-py. It allowed developers access to podman features without having to port their whole scripts. From there I suspect there are areas where new development followed the old form even if it didn't make as much sense.
The flexibility of args / kwargs have always been very Pythonic and pre-date type hinting.
There was a problem hiding this comment.
I see, thanks for your answer. Would there be any value in a PR for moving most of the kwargs to typed parameters for at least images.build? I've tried to do it but my changes do break existing code (I made all the strings that are used for paths become a pathlib.Path, so the argument has to be Path("something") instead of just "something").
| @@ -144,6 +145,19 @@ def test_build(self): | |||
| self.assertIsNotNone(image) | |||
| self.assertIsNotNone(image.id) | |||
|
|
|||
| def test_build_cache(self): | |||
There was a problem hiding this comment.
more tests are probably needed to ensure that the defaults are passed, and images.build also accepts the other parameters
There was a problem hiding this comment.
There are defaults like those above, and then there is dockerfile that is generated randomly... maybe this is something that should be done in a unit test?
There was a problem hiding this comment.
There are defaults like those above, and then there is dockerfile that is generated randomly... maybe this is something that should be done in a unit test?
I think you can do all through integration tests by inspecting the request that are passed. if you want I can write you some pseudo-code to help understand how I would design it
There was a problem hiding this comment.
Yes please, I’m not familiar with this kind of stuff… how would I inspect what is requested? I’ve seen how it is mocked in unit tests, is it related?
There was a problem hiding this comment.
Yes please, I’m not familiar with this kind of stuff… how would I inspect what is requested? I’ve seen how it is mocked in unit tests, is it related?
Actually my bad, I was looking into many things at the same time and got confused. You are correct, unit tests is the place where you check you function. Also yes, you said it right, you need to mock a request and test that your image build calls the parameters correctly in the default/non-default cases.
There was a problem hiding this comment.
more tests are probably needed to ensure that the defaults are passed, and images.build also accepts the other parameters
About the other parameters, you mean that, besides the new defaults, there should be a test should that uses all of them and inspects the request? In that case, there is another issue: while experimenting with the parameter-argument conversion (see above), I discovered that _render_params parses the "remote" key, it is not documented, it can not be used alone because _render_params requires either "path" or "fileobj" (and both are meaningless if remote is used), but other than that the build function is able to handle this, because body is initialized with None, and when all the if/elif fail it stays None which is right for remote. But if dockerfile is None, then it is generated randomly in _render_params and this will be yet another issue: "If the URI points to a tarball and the dockerfile parameter is also specified, there must be a file with the corresponding path inside the tarball" from https://docs.podman.io/en/latest/_static/api.html#tag/images/operation/ImageBuildLibpod
I understand this is going quite out of the "cache" issue scope, I apologize in advance.
There was a problem hiding this comment.
I've added a unit test that checks the default parameters for images.build, with it also I corrected the mock URL for two other test cases
This commit fixes issue containers#528 by adding a default value to parameters layers and outputformat. This change aligns the behavior with podman-remote. Signed-off-by: Federico Rizzo <[email protected]>
Signed-off-by: Federico Rizzo <[email protected]>
Replace function "default" inside _render_params in images_build.py with dict.get (kwargs.get) default value. Signed-off-by: Federico Rizzo <[email protected]>
Signed-off-by: Federico Rizzo <[email protected]>
Signed-off-by: Federico Rizzo <[email protected]>
This commit fixes issue #528 by adding a default value to parameters layers and outputformat. This change aligns the behavior with podman-remote.
@inknos take a look, I've also added a test for this PR