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

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

Merged
merged 1 commit into from
Jun 26, 2024

Conversation

PlugNPush
Copy link
Contributor

@PlugNPush PlugNPush commented Jun 26, 2024

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

p12tic commented Jun 26, 2024

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!

@PlugNPush
Copy link
Contributor Author

PlugNPush commented Jun 26, 2024

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

p12tic commented Jun 26, 2024

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.

Copy link
Collaborator

@p12tic p12tic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thanks!

Could you please add a newsfragment with a description of what this fixes? This is just one line file in newsfragments directory, see newsfragments/README.md for an explanation and latest release on how other release notes look like.

Also, could you combine the commits into one? Whole PR is semantically one change with tests, so there's no need to split it across multiple commits to show how the code evolved until it arrived to correct conclusion.

Signed-off-by: Michaël Nass - PlugN <[email protected]>
@PlugNPush
Copy link
Contributor Author

All done! @p12tic

@PlugNPush PlugNPush requested a review from p12tic June 26, 2024 18:08
@p12tic
Copy link
Collaborator

p12tic commented Jun 26, 2024

Thanks!

@p12tic p12tic merged commit d9a3572 into containers:main Jun 26, 2024
8 checks passed
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.

1.1.0 (and perhaps above) broke the use of Containerfiles Broken support for Containerfile
3 participants