-
Notifications
You must be signed in to change notification settings - Fork 106
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
Refactor docker build/push workflow action #11653
Conversation
Create env var for CERN registry
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.
I need more info about provided logic, see inline.
CERN_REGISTRY: registry.cern.ch | ||
run: | | ||
echo "Building service: ${SERVICE_NAME}, with tag: ${PYPI_TAG}" | ||
svn checkout https://github.com/dmwm/CMSKubernetes/trunk/docker/pypi/${SERVICE_NAME} |
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.
Why do we need svn
to checkout github repo? Why not to use git
for that?
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.
For this question, please refer to: #11639 for full context.
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.
Alan, even though ticket lists that you swap curl
with svn
it does not provide a reason, and neither address why you use curl/svn
to get git repo files, why not to use (tool designed for that) git
? Please note that in all CMSKubernetes yaml file I always relies on git, e.g. RUN git checkout tags/$TAG -b build
see https://github.com/dmwm/CMSKubernetes/blob/master/docker/dbs2go/Dockerfile#L33
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.
@todor-ivanov has done the research on the svn
util. My understanding is that with svn
, we can fetch a sub-directory of the repository, while git
does not provide this capability and we would have to clone the whole repository for each of the 11(?) services that we have setup in the actions workflow.
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.
ok, thanks for providing the reason. Said that, this line is not doing what the comment is saying, i.e. it will fetch latest subdir and not a particular tag. I do not know svn
enough to answer if it can fetch proper tag of sub-dir, but curl
cat fetch tarball of particular tag, and git
indeed will fetch entire repo for that tag. So, we should fix either svn
or switch back to curl
to fetch exact tag of the repo.
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.
Valentin, the CMSKubernetes code that we check out isn't really tagged (we always fetch what is in HEAD).
The PYPI_TAG is indeed used, but then in the scope of WMCore (used for the tag in pypi and the docker build argument).
Having said that, I would say it works as expected and there is nothing to be changed here.
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.
with svn, we can fetch a sub-directory of the repository, while git does not provide this capability and we would have to clone the whole repository for each of the 11(?) services that we have setup
Yes this is the exact reason.
So, we should fix either svn or switch back to curl to fetch exact tag of the repo
SVN is fully capable of fetching particular tag.
echo "Retrieved Dockerfile with content:" | ||
cat Dockerfile | ||
echo "Sleeping 5min to ensure that PyPi packages are available..." | ||
sleep 300 |
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.
why do we need to sleep, what do you mean that PyPi packages are available? Once you download them they should be available and I don't see any needs for sleep. Why 300 and not any other number?
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.
this we discussed when we were commissioning the build workflow. There is a delay between publishing a package to the PyPi repository and fetching it. This 5min is a fair enough commitment that we came up with in the past and which allows us to use the PyPi package when building the docker image.
Jenkins results:
|
@vkuznet @todor-ivanov if there is no further comments and or concerns, I would like to get it merged because deployment is scheduled for tomorrow and we still need to have another testbed upgrade this afternoon. |
@amaltaro , I provided my concerns, and it is up to you to disregard them or not. I am not fan of having sleep commands as they can lead to incorrect results, the publishing to pypi should be separated from build in my opinion. The |
@vkuznet Valentin, I will get it merged then, but I am happy to keep these discussions ongoing and in case we decide to change things, we can follow up with a new issue and/or PR as well. However, let me add a few more content on your comments:
|
Just FYI, we have finally recovered from the failure mode between |
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.
Sorry for the late review - it all looks good
Thanks @amaltaro
Fixes #11646
Status
ready
Description
I created a new repository and tested these changes to make sure that we can now build at least
reqmgr2ms-unmerged
package, which was failing for the last couple of tags.Summary of changes is:
docker/build-push-action@v1
, instead, push the docker image in the same step as we build it.SERVICE_NAME
job wide environment variable with the WMCore component to be built/published.CERN_REGISTRY
step wide environment variable (all my attempts to set it job wide failed, either with onlyIs it backward compatible (if not, which system it affects?)
YES
Related PRs
Resolving unresolved issues from:
#11638
#11651
External dependencies / deployment changes
None