-
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
Refactor Unified configuration; remove unused references #11770
Conversation
And with the script provided in this PR, I have just updated/cleaned the Unified configuration in testbed, as can be seen in: |
Jenkins results:
|
Jenkins results:
|
Jenkins results:
|
Oups, let me fix those unit tests before any code review. |
Jenkins results:
|
test this please |
Jenkins results:
|
unit test failure is unrelated. This PR is ready for a review. |
And to not delay it any further, I have just updated the Unified configuration in central production to have the latest updates provided in the Unified repository. |
bin/adhoc-scripts/injectUnified.py
Outdated
} | ||
|
||
if __name__ == '__main__': | ||
url = 'cmsweb-testbed.cern.ch' |
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 do not hard-code options, it is better if you'll use sys.argv
to pass around it as an input option. If it is not provided then you can fall back to specific default (like this one)>
bin/adhoc-scripts/injectUnified.py
Outdated
|
||
print(f"Updating unified configuration in {url} with content: {pformat(DOC)}") | ||
conn = http.client.HTTPSConnection(url, | ||
cert_file=os.getenv('X509_USER_PROXY'), |
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.
before making a call, please add block to check this env parameters, and if they are empty please provide meaningful message. The script should exit with non zero status in this case.
bin/adhoc-scripts/injectUnified.py
Outdated
print("Response status: %s\tResponse reason: %s" % (resp.status, resp.reason)) | ||
if hasattr(resp.msg, "x-error-detail"): | ||
print("Error message: %s" % resp.msg["x-error-detail"]) | ||
sys.exit(1) |
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 you exit here your connection will remain open, please refactor the code with with
clause to automatically close the connection
Jenkins results:
|
Remove Unified update from AuxCacheUpdateTasks thread Add schema and validation to the unifiedconfig endpoint Standardize error messages new line separating AuxValidation_t unit tests Use safe.kwargs in the REST server
Apply Valentins suggestions to injectUnified.py
fix conflict in unit test adjust unit tests AuxValidation_t unit tests
Jenkins results:
|
I actually planned to remove that script under "bin/adhoc-scripts/injectUnified.py", but it turns out we might actually need it for our testX kubernetes clusters, so I decided to keep it around and I have also applied the suggestions that you made, @vkuznet . Initial description has been updated and it's now ready for another review. The unit test failure is unrelated - and actually unstable. |
I just noticed that the Unified configuration in testbed and production had all the previous parameters (in addition to the supported ones). After cross-checking the default configuration in WMCore against the P&R gitlab's one, I pushed the up-to-date data structure into testbed and production, from vocms0193 (latest agent):
|
Fixes #11695
Status
ready
Description
List of changes is:
unifiedconfig
endpoint. Plus moved a few other things around.validateUnifiedConfig
to validate user input when creating and/or updating the Unified configuration in central CouchDBIs it backward compatible (if not, which system it affects?)
YES
Related PRs
None
External dependencies / deployment changes
None