-
Notifications
You must be signed in to change notification settings - Fork 39
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While I do understand that you want to make it easy to use COPY command I am bit against this move for a different reason: you endup producing temp files (dockerfile) inside the source tree.
Testing framework should not produce leftovers (untracked or modified) files after running and this change modifies that.
I think that the correct approach for this is to template paths used inside COPY commands when you want to do that.
Maybe someone has a better idea about how to address that aspect.
I may be misunderstanding but I can't think of a way to use templating in COPY instructions within a Dockerfile that would help. Would that be more acceptable if there were a cleanup playbook to deal with that leftover Dockerfiles? |
As the |
I am a bit biased here because I really dislike the "let me build the image right before the test"-parts of the molecule. For all our tests, we maintain our own docker images that molecule then consumes directly. That being said, I do see how the One "clean" solution I see here is to copy the whole scenario directory into some temporary location, build the image there, and then delete the whole thing. Building images in the scenario directory is another alternative that has the potential to leave the temporary files in the project folder, which is not something I would like to see. I already hate the molecule for creating the |
Sure, but then the files to COPY would live in the scenario directory, while the docker build is done using MOLECULE_EPHEMERAL_DIRECTORY as build path so the COPY would not work (as explained by @tadeboro). That's what I tried to explain in my previous comment but was maybe unclear |
I realize having a new ootb playbook might create concerns but here is an example cleanup I had in mind (didn't test it):
|
Cleanup is no always called, especially when doing development. This means that relying on it for clearing tmp files is not really the best idea. Have you tried to use the templating directly into the Dockerfile and to avoid changing the directory where we put it? I am confident that this would work. If it works an PR would still be useful that would exemplify how to use COPY with that path, so other users will know how to write Dockerfiles that work well with molecule. |
Just did the test and it fails as I expected due to the file being outside of the build context:
here is the molecule.yml:
And the Dockerfile.j2:
Sanity check:
|
@alxgomz Can you please show the generated Dockerfile made by molecule, or at least extract the COPY line from it? I am curious what went wrong. I was not able to find any place that states that you cannot use COPY with full paths. Do you have any custom I wonder if the error may be caused by the weird destination instead of the source. Maybe you could try to copy to /root just to see if it does work. |
@ssbarnea From the docker docs at https://docs.docker.com/engine/reference/builder/#copy:
So templating the |
below is what the template expanded to:
No .dockerignore. The key really is with the fact files need to live within the build path. You can easily double check it by trying manually building an image usingdocker CLI only. For example using COPY ../filetocopy /tmp will failed whatsoever.
This is what @tadeboro pointed earlier today. |
I came up with xlab-steampunk@7cd45f2 as an alternative way of solving this issue. That additional task copies over the contents of the scenario directory, which should allow users to write their Dockerfiles as if the build will be executed from the scenario directory. I did not test this because it is really late here. @alxgomz, will you be so kind and test this alternative approach and make sure it solves your problem? Thank you in advance. |
I was not super keen of copying files in a bulk, but I have to say that it worked quite nicely for me. |
Thinking twice, i can see one down side in using file copy without setting the build path. I have a playbook with multiple roles, and they all use the same Dockerfile.j2 file and the same filetoCopy within it. |
I would say that this use case can be solved in a cleaner way by using prebuilt docker images. Just build the image before running the molecule and save time when running scenarios. I do see the value of being able to build scenario-specific images on the fly, but image that is shared across different scenarios feels like something that is out of scope for molecule. |
The commit I created is just a proof-of-concept implementation. We can always add some validation to make sure we do not override anything important or move the whole scenario directory into a sub dir in the ephemeral directory. I just wanted to see if the alternative implementation would work.
I was hoping that if we decide that this is the way we want to go, you will create a new one or update the existing one ;) |
Ah just seeing your comment now after I pushed an update to my PR. Whichever method is chosen it would require a bit of documentation, which I'm not sure where would be best suited. @tadeboro no worries, I'll take care of the PR once we know what is the right way forward (and thanks for helping out here). |
Do we know what approach we will take? Because right now, the changes in this PR implement both of them as far as I can tell. |
Fix bug introduced by #51 where ansible would choke when path is not specified and pre_build_image is false.
Fix bug introduced by #51 where ansible would choke when path is not specified and pre_build_image is false.
PR intend to fix #50 by changing the build path to either:
There may be implications I didn't think of but from the basic tests I've made "it just work on my laptop" :)