containers / podman-compose

a script to run docker-compose.yml using podman
GNU General Public License v2.0
4.85k stars 463 forks source link

Restore support for alt dockerfiles (removing hardcoded "Dockerfile") #978

Closed PlugNPush closed 1 week ago

PlugNPush commented 1 week ago

Resolves #977 Resolves #929 This commit restores setting a build directory as a string and looking for all available dockerfile alt names. I strongly believe this is a bugfix, not modifying a compatibilty feature, as this was working that way in the older version 1.0.6.

The code for checking all the dockerfile alt names is already there, but "Dockerfile" was hardcoded in the normalize function, instead of being set to None to allow further alt dockerfile names search later in build_one().

build_one() already supports dockerfile being set to None (and that is visibly supposed to be that way).

With this PR, having a podman-compose.yaml with: build: . No longer throws "No Dockerfile found in CTX" when using a Containerfile (or other alt name).

p12tic commented 1 week ago

Thanks a lot for the PR. It looks good from the first sight, didn't look in depth yet.

We will need tests that cover all 4 cases:

Therefore it's not appropriate just to change existing tests, we will need additional cases. Hopefully this is mostly copy-paste and can be done without much effort.

Thanks again!

PlugNPush commented 1 week ago

Thanks a lot for the PR. It looks good from the first sight, didn't look in depth yet.

We will need tests that cover all 4 cases:

* `build: .`

* build dictionary but not dockerfile

* build dictionary with `dockerfile: Dockerfile`

* build dictionary with `dockerfile: Dockerfile.custom`

Therefore it's not appropriate just to change existing tests, we will need additional cases. Hopefully this is mostly copy-paste and can be done without much effort.

Thanks again!

All these cases are already included in test_normalize_final_build.py, have a look at the changes :) They used to expect "dockerfile": "Dockerfile" in every test case, which is incorrect when trying to support alt dockerfile names.

p12tic commented 1 week ago

All these cases are already included in test_normalize_final_build.py, have a look at the changes :) They used to expect "dockerfile": "Dockerfile" in every test case, which is incorrect when trying to support alt dockerfile names.

You're right. Turns out that the caveat of didn't look in depth yet was worth writing.

PlugNPush commented 1 week ago

All done! @p12tic

p12tic commented 1 week ago

Thanks!