-
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
Add minimal CouchDB container for WMAgent #1409
Conversation
2e1a1d2
to
4d68b80
Compare
Remove anything unnecessary Remove local-upstream-minimal.ini
4d68b80
to
cd32771
Compare
… broken tag build parameter
… security string checks from manage script.
@todor-ivanov Should we add a mechanism to read the couch user/pass from a config file ? Read my comment from: |
Hi @khurtado we should indeed. I am workign on that now, and hopefully will provide the code later tonight. |
@todor-ivanov Sounds good! I will test on my end once you are done but let me know if you need help with anything. |
…CH}_SECRETS_FILE && Start couchDB from run.sh
@todor-ivanov I was testing the options in the script and this one didn't work for me:
|
hi @khurtado
You need to provide the WMCore tag from which the relevant
|
…nly admin section Typo
ca5ddbc
to
8b2c2ab
Compare
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 Code is looking good to me, I left a few comments. Just one thing overall, this PR is in general not covering non-cmst1 users, right? I vaguely remember CERN IT was going to be involved in this, security wise (regarding including uuids and usernames of all three users Agents use)
I updated the documentation suggestion with the updatecouch output in your previous comment.
@todor-ivanov Just a reminder of this review for when you get a change to look at it |
Co-authored-by: Kenyi Hurtado <khurtado@nd.edu>
Hi @khurtado I have merged all your suggeted changes to the current PR. I also addressed your comments and also added the needed push and pull parameters to the scripts in order to run upload and run this image to/from the cern registry. Please take another look. |
Forgotten extra repository variable
decc667
to
14e560e
Compare
@todor-ivanov The push & pull code in the new commit looks good to me, but since the image name has changed I think now the documentation instructions mentioning
would be:
Is that right? Could you please change that to be consistent with the new name? |
hi @khurtado, the environment variable I kept there on purpose just to stay consistent with the structure of the rest of the containers. And also to have the placeholder once we start having those database accesses properly set with the correct database level access rights for the accounts (which, IIUC, we are to do it in some near future). For the command for running/accessing and connecting to the container nothing has changed. The image name has changed, but the container name is still
is still valid. |
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.
Thank you Todor! I just marked the requests as resolved and approved the PR.
ENV COUCH_LOG_DIR=$COUCH_CURRENT_DIR/logs | ||
ENV COUCH_DEPLOY_DIR=/usr/local | ||
ENV COUCH_ENV_FILE=$COUCH_DEPLOY_DIR/deploy/env.sh | ||
ENV COUCH_SECRETS_FILE=$COUCH_ADMIN_DIR/CouchDB.secrets |
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.
If this is var not being used anymore, it can be removed.
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 comment is marked as resolved, but I see no follow up. It might be confusing to leave an unused secret reference in this docker file. I would suggest to remove this COUCH_SECRETS_FILE
line?
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 was discussed with @khurtado in another comment/thread as well ( I cannot remember already which exactly), but we have agreed to keep the variable as a placeholder for the sake of consistency with the MariaDB container and because we do plan to fix this single user problem in the future.
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 would leave it commented out, but that's Okay.
thanks @khurtado |
@amaltaro Do you want to take a final look at the PR before merging it? |
Yes, I plan to look into it by tomorrow. Apologies for the delay. |
@amaltaro would you be able to look into this before the start of this long weekend. Thanks in advance |
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, it's looking good in general, however I did leave many comments/questions/requests along the code.
In addition to that, please update the initial pull request description.
cacert_file = /data/certs/servicecert.pem | ||
ssl_certificate_max_depth = 10 | ||
verify_ssl_certificates = false | ||
fail_if_no_peer_cert = false |
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 configuration is new w.r.t. the actual one. I would drop it unless there is a strong reason to keep it.
cacert_file = /data/certs/servicecert.pem | ||
ssl_certificate_max_depth = 10 | ||
verify_ssl_certificates = false | ||
fail_if_no_peer_cert = false |
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.
same here
@amaltaro I have addressed all your comments please take another look. |
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.
Thank you for the prompt turnaround, Todor. I left a few more comments along the code.
In addition, please add a few more details to the PR description. For someone that didn't work on this, it's not clear where docker images are uploaded; what the assumptions are (secrets? accounts?); current couchdb version; any relevant changes w.r.t. the current couchdb setup.
ENV COUCH_LOG_DIR=$COUCH_CURRENT_DIR/logs | ||
ENV COUCH_DEPLOY_DIR=/usr/local | ||
ENV COUCH_ENV_FILE=$COUCH_DEPLOY_DIR/deploy/env.sh | ||
ENV COUCH_SECRETS_FILE=$COUCH_ADMIN_DIR/CouchDB.secrets |
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 comment is marked as resolved, but I see no follow up. It might be confusing to leave an unused secret reference in this docker file. I would suggest to remove this COUCH_SECRETS_FILE
line?
@amaltaro take another look |
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.
Thanks Todor, it looks good to me.
fixing dmwm/WMCore#11312
This PR adds all necessary changes for setting up a WMAgent couchdb container.
registry: registry.cern.ch
repository: [cmsweb/wmagent-couchdb](https://registry.cern.ch/harbor/projects/1771/repositories/wmagent-couchdb)
It is ready to use.