-
Notifications
You must be signed in to change notification settings - Fork 107
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
move ckey/cert functions to Utils.CertTools #11101
Conversation
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
@amaltaro this PR is ready for review. One note though. The py3k lint checks fail with round built-in referenced which is explained here https://stackoverflow.com/questions/59533912/what-is-the-alternative-to-built-in-round. I think we should not degrade about py2 compatibility and the pylint documentation suggests that it is only for backward compatibility, and therefore it should be ignored or it is better to adjust pylint configuration to skip this test. |
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 @vkuznet
The code looks good to me.
And also thanks for fixing two of the old standing pylint warnings.
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 looks good to me, Valentin. However, given you are already on this, could you please update the same in this module:
https://github.com/dmwm/WMCore/blob/master/src/python/WMCore/MicroService/Tools/PycurlRucio.py#L32-L38
?
Keep in mind there is an import that we can likely remove as well.
Jenkins results:
|
Alan, I corrected |
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 Valentin, it looks good to me. Regarding pylint 3k, Erik and I had a brief chat and we also think it could be removed (AFAIR it checks whether our py2 code is compatible with py3, in some sense).
Partially Fixes #11098 (moving cert/ceky function will allow to share code between MicroServices and DBS services)
Status
ready
Description
Move functions
ckey
andcert
intoUtils.CertTools
which is generic place to import from other places. This will allow to share and use this code in different Services, like MicroServices and DBS.Is it backward compatible (if not, which system it affects?)
YES
Related PRs
See #11099 , if applied then I can safely remove these functions from #11099 and use common code.
External dependencies / deployment changes