-
Notifications
You must be signed in to change notification settings - Fork 108
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
Script to update pileup documents in MSPileup via REST API #11868
Conversation
Jenkins results:
|
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, it is a good idea to use service itself to update docs. I left few comments in a code and here is my additional feedback:
- based on a scope/use-cases where this script will be used you may consider making it fully independent from WMCore dependency. Usage of pycurl is convenience (which brings lots of dependencies) rather then a necessity since Python standard library provides ability to handle HTTP. Here I meant python http libs, like urllib, rather 3rd part requests one. The advantages of having pure python is that you don't require on your node anything except python and therefore can quickly run the script without having WMCore or other libs around. Again, it is observation rather a requirement.
- the same applies to choice of logging, if script can be used in shell simple print will do the work without adding logging module
- finally, the main issue here is security concern. Even though all our services requires authorization by default all people in productiion operator group will have ability to run it without any issues. Depending on our view of the internal database it can be acceptable or not. I just want to raise this awareness.
ckey=certDict['key'], cert=certDict['cert']) | ||
|
||
logger.debug("Response: %s", resp) | ||
if resp and not resp.get("result", []): |
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.
please add appropriate docstring explaning response structure
logging.basicConfig() | ||
|
||
# setup proxy/cert | ||
cert = os.getenv('X509_USER_CERT', '') |
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.
You may consider for user convenience first check proxy and use it, if it does not exist check user key/cert and make default to $HOME/.globus/userkey.pem
and $HOME/.globus/usercert.pem
. This way you'll cover all 3 use-cases, proxy, user env, and default user pem files.
hdict = {'cert': cert, 'key': key, 'pycurl': True} | ||
|
||
fin = opts.fin | ||
with open(fin, 'r') as istream: |
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.
As far as I'm aware now it is "required" to specify in open encoding and it is desired to specify errors, e.g. with io.open(fin, 'r', encoding='utf8', errors='ignore') as f:
I would suggest add this.
Superseeded by #11872 |
Fixes #11867
Status
ready
Description
Script that uses the MSPileup REST API to fetch all of the pileup documents, update each of them with data provided via a JSON file, then update those documents back in MSPileup (through a PUT http call).
It has been tested and all the testbed and production pileup documents now have the
customName
attribute, with an empty string as value.Is it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None