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

Updating the Bats version and changing the travis.yml #14

Merged
merged 3 commits into from
Jul 8, 2019

Conversation

anagno
Copy link
Contributor

@anagno anagno commented Jul 7, 2019

This merge request:

  • resolves Bats new version #13. The new version of Bats is compatible with this image (I was able, without any changes to run the tests, and they were green). So it is probably a good idea to update the README.md to point to the new version.
  • Changes the travis.yml. I really liked the changes in Add multiarch support #12, but there was some duplication of code in the dockerfiles. So I tried to refactor it and eliminate the code duplication. This was achieved by injecting the necessary dependencies/binaries during the build process in the travis-ci. When building in a native arm machine (such as a raspberry pi), docker will resolve automatically the correct images. So we only need a single image in general. The only reason we needed the other dockerfiles was because we had to inject the correct binaries for cross compiling.

The new travis.yml will build all commits from all branches and the name for the images are BRANCH_NAME-COMMIT_HASH. If we tag the repo, a new image will be build again but this time the name will be TAG_ΝΑΜΕ (for an example see https://cloud.docker.com/repository/docker/anagno/light-baseimage/tags).

I hope it is useful.
If something is not clear (or wrong), please let me know and I will try to explain it (fix it) :)

Improving the travis.yml to inject the necessary dependencies to
cross build our images. This leads to less duplication of code,
since our Dockerfile do not need to be replicated.
@BertrandGouny
Copy link
Member

Hello,
thanks for your PR,
i noticed that several files are delete, is that intentional ?

@anagno
Copy link
Contributor Author

anagno commented Jul 7, 2019

Hello,

Yes. All the files were copies of the original Dockerfile with a single line changed (the line to copy the correct qemu binary). This line is only necessary when you cross building the image. If you natively build the image in arm, this line is not necessary. So I am injecting this line via jenkins (line 50 to 56 in travis.yml). I think it leads to less duplication of code :)

KR,

Vasileios

@BertrandGouny
Copy link
Member

Ok Thanks,

This project don't use travis, so please let it possible to build image with an other way, with the makefile for instance.

I ok to let the travis config for example, and if used by any forks, but I would like to have the review of @ndanyluk for the travis changes

@anagno
Copy link
Contributor Author

anagno commented Jul 8, 2019

Hello,

If you do not want to use travis-ci.org/ travis.yml then I would suggest not integrating it at all. The current version of the travis.yml (the one that already exists in the branch) will try to push to osixia/base-light-baseimage. I did the same mistake, and you might have already received some notifications that someone without authorization tried to push in docker hub (sorry :) ). In general CI is very specific per repo, so I would leave it to each person to implement their workflow.

As far as it concerns cross building, my only “objection” (not really an objection but I could not find a better word) is that the previous approach will not work in native builds (or at least I think it will not work -- please correct me if I am wrong). The changes we are introducing are a virtualization layer. We are registering a virtualization layer and we are forcing the machine to build an arm image (and usually that is why it takes so longer) . But these changes are not necessary in native builds. So the changes are valid only in the case you are trying to build an arm image in an x86_64 machine. They are not generic for all arm builds.

Furthermore the duplication of the code is probably is not a good idea. My docker cluster has mainly Raspberry Pies and very few x86_64 machines. So I am also trying to use ARM images. Making this repo ARM compatible is the first step to make the openldap image ARM compatible. This duplication is probably ok in the case of this specific dockerfile, but in the case of the openldap image (which has a bigger dockerfile) it becomes more difficult to maintain, if we follow the same pattern.

Currently docker does not support including a dockerfile inside another dockerfile (e.g. moby/moby#735). The most common pattern I have seen to reduce code duplication and handle these cases is either to write scripts that auto generate the final dockerfiles (e.g. https://github.com/nextcloud/docker) or inject the necessary changes via the CI.

In my projects (e.g. https://github.com/anagno/docker-SelfServicePassword) I usually develop on my desktop (which is x86_64) and when I am satisfied I commit on the repo and let the continuous integration to take it over and cross-build the images. If for example travis-ci.org supports native arm builds, then these changes are not necessary anymore and I will only have to change the travis.yml file.

So in conclusion, it depends on the workflow you want to follow. If you prefer I can extend the make file to generate the files that I deleted. You can even disregard my pull request. It was just a suggestion :) What is your opinion?

KR,
Vasileios

@BertrandGouny
Copy link
Member

Thanks for the detailled explanations,
will give a try to travis :)

@BertrandGouny BertrandGouny merged commit 80e6552 into osixia:release-1.2.0 Jul 8, 2019
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.

2 participants