Update GH action for docker image to use a build argument #11638
Conversation
|
Jenkins results:
|
There was a problem hiding this comment.
Thanks for adding this options @amaltaro
Please take a look at the single comment inline. And just for documentation purposes, here is my previous comment about the so suggested change #8797 (comment).
Sorry for repeating myself.
| @@ -25,12 +25,12 @@ jobs: | |||
|
|
|||
| - name: Build image | |||
| run: | | |||
| PYPI_TAG=${{steps.get-ref.outputs.tag}} | |||
| curl -ksLO https://raw.githubusercontent.com/dmwm/CMSKubernetes/master/docker/pypi/${{inputs.wmcore_component}}/Dockerfile | |||
There was a problem hiding this comment.
Please consider either fetching the whole CMSKubernetes project area through git and position in the proper directory for the current build. Or use svn and fetch only the portion of the CMSKubernetes project area that relates to the current build. So we avoid further complications and workarrounds just because we stick to a single Docker file builds, while the Docker system in general contains the current working director in its search path and and uses it.
There was a problem hiding this comment.
With the separation of dockerfiles, this is no longer an issue. It is also not clear to me what the svn dependency would incur here, so I'd rather leave it for a future development, in case we deem it necessary.
There was a problem hiding this comment.
hi @amaltaro
With the separation of dockerfiles, this is no longer an issue.
This is not exactly correct. In order to separate the images you had to move things/files around, which is about to cause other issues at runtime. While the separation of images is a good thing as I said multiple times, this is in no way a substitute of a must fix here. And such separation should definitely be done without the need of shuffling an already established construction of the original image, so that this separation does not affect steps further down the line and causing effects most of the times revealing themselves at runtime but not at build time.
It is also not clear to me what the svn dependency would incur here
And here is the full list of dependencies for svn:
apt-cache show subversion
Package: subversion
Version: 1.10.4-1+deb10u3
Installed-Size: 4809
Maintainer: James McCoy <jamessan@debian.org>
Architecture: amd64
Depends: libsvn1 (= 1.10.4-1+deb10u3), libapr1 (>= 1.5.0), libaprutil1 (>= 1.3.2+dfsg), libc6 (>= 2.4), libsasl2-2 (>= 2.1.27+dfsg)
Merrily 5 libraries. And on a standard Debian based system, the full set of dependencies (excluding libc6 and libsasl2, which must have been satisfied already due to their multiple forward dependencies), the added size is exactly 9946 kB. See bellow:
apt-get install subversion
...
The following additional packages will be installed:
libapr1 libaprutil1 libserf-1-1 libsvn1
...
0 upgraded, 5 newly installed, 0 to remove and 322 not upgraded.
Need to get 0 B/2,658 kB of archives.
After this operation, 9,946 kB of additional disk space will be used.
I do not see a reason why we should make our life harder by striping the capabilities of the whole containerization system only because we decided not to sync the whole needed project area in our CI/CD pipeline. This implies restrictions on our work. Just because the capabilities of the CI/CD system differ from the manual builds and the standard way of how Docker is used, we doom ourselves to indefinite remembering of an obstacle, forcing us to create multiple future workarounds. We can see how this already stands in the way of the WMAgent image creation. The proper way of fixing the issue is not every time we stumble on it to create a workaround in the component's deployment which suffers from the obstacle we have nailed here, but rather fixing the obstacle itself once and forever. Moreover, provided the cost for such a fix is less than 10 MB.
There was a problem hiding this comment.
No, that is not correct! I simply move files from the wmagent directory over to the wmagent-base one, keeping those files added under the same directory. So I fail to understand why you say it will not work and why there is yet a fix that must be done.
In addition, it is not because we started with those scripts under wmagent dir that we need to end with them in there. Keep in mind that this was just an initial work and there is still many developments before we get to something more palpable.
There was a problem hiding this comment.
No, that is not correct! I simply move files from the wmagent directory over to the wmagent-base one, keeping those files added under the same directory
No this is not correct. You do change destination of files. And this even though may go well at build time will break at runtime. Please take a look at my other review.
|
@todor-ivanov if you consider it important to make changes to the CD pipeline, please make a ticket and we deal with that in the future. The reason I am reluctant to make any changes that are not really required at the moment is:
I also tested the build process of both wmagent-base and wmagent images, nothing failed during build time and I also checked a few files and directories inside the image, to me it looks Okay as is. I am even considering in rolling back the changes I made for the TAG variable assignment in this PR, I do not know if that syntax is the correct one and I believe it too troublesome to be tested. If you feel like we should change that back, I am happy to. Otherwise we will have to try this fix. |
|
@amaltaro, I have just checked, and actually Let me suggest another, much simpler solution to #8797 |
|
Jenkins results:
|
| sed -i -e "s,ENV TAG=.*,ENV TAG=${{steps.get-ref.outputs.tag}},g" Dockerfile | ||
| echo "Building service: ${{inputs.wmcore_component}}, with tag: ${PYPI_TAG}" | ||
| svn checkout https://github.com/dmwm/CMSKubernetes/trunk/docker/pypi/$SERVICE | ||
| cd $SERVICE |
There was a problem hiding this comment.
@amaltaro is this variable $SERVICE exported from somewhere up?
There was a problem hiding this comment.
I meant to fix it yesterday evening. Sorry for the ill review request! Updating it in a second.
properly define the pypi_tag remove the SERVICE variable
|
Jenkins results:
|
Fixes #11646 (while before it was supposed to address #8797)
Status
not-tested
Description
Instead of replacing string in place in the Dockerfile (sed command), we can use the docker build argument to pass the tag that we want to be built within github actions.
It depends on the 2nd commit provided in this PR:
dmwm/CMSKubernetes#1394
Is it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
CMSKubernetes: dmwm/CMSKubernetes#1397