-
Notifications
You must be signed in to change notification settings - Fork 42
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
Fix WMAgent rpm dcoker image #1362
base: master
Are you sure you want to change the base?
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.
@todor-ivanov Todor, I assume you wanted to update this deployment model to the recent changes we had in WMCore, so I left a few comments along the code for your consideration.
@@ -51,8 +56,8 @@ mkdir -p $DEPLOY_DIR || true | |||
cd $BASE_DIR | |||
rm -rf deployment deployment.zip deployment-${DEPLOY_TAG}; | |||
|
|||
set -e | |||
wget -nv -O deployment.zip --no-check-certificate https://github.com/dmwm/deployment/archive/$DEPLOY_TAG.zip |
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, if I pass master
as the DEPLOY_TAG, this line seems to work just fine. And so it does for an old HG tag. Do we really need to change this github url?
@@ -8,6 +8,7 @@ RUN adduser -u 31961 -g 1399 cmst1 | |||
RUN install -o cmst1 -g 1399 -d /data/srv /data/admin /data/certs | |||
RUN chown root:1399 /data | |||
|
|||
ARG WMA_TAG=None |
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.
Where does WMA_TAG
come from? Shouldn't this be using the ENV keyword instead of ARG?
@@ -1,4 +1,4 @@ | |||
FROM cmssw/cmsweb:20221130-stable | |||
FROM registry.cern.ch/cmsweb/cmsweb:20221130-stable |
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 think we have a more recent cmsweb image. I'd suggest to update it to the latest.
Hi @amaltaro thanks for looking into this PR as well. This one was actually just for me, to keep track of all the changes I had to do in order to make this old deployment method work and then use it as initial investigation on how to continue with the Python based docker image. If we want we may finalize this of course. Up to us. |
Do NOT merge Yet.
The current PR is intended only to fix the old RPM based Docker image. so that we can test it and reuse bits of it in the new Pypi based setup.