-
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
Add customName to JSON data and adhoc script #11769
Conversation
Jenkins results:
|
Alan, Todor, I made initial changes which are ready to review. I see one WQ test failing which I think is not related to this PR. Please review and provide your feedback of what else need to be done. |
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.
Valentin, I left a few comments for your consideration along the code.
The missing piece here is (from the ticket description):
"""
fetch all the MSPileup documents from MongoDB and update them to have the customName field as well
this has to be done for integration and production (perhaps for development databases as well?)
"""
but I think it would be premature to make it right now and I'd rather first see some extra developments of this feature going in.
Jenkins results:
|
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.
Valentin, did you miss the ad-hoc script?
In addition, we will need to update all the documents in MongoDB with the new field, but before that can be performed, I think we need to deploy the new pileup document schema implemented in:
#11765
is that correct?
@amaltaro , which ad-hoc script? I reverted the changes to ad-hoc scripts per your original feedback. I'm confused, please clarify it further. Regarding MongoDB documents, again I'm confused. In my view there is no need to update any MongoDB documents per-se since new attribute field is optional, i.e. if old document does not have it is fine. For new documents it may be present. Please clarify your comment on this subject as well. |
Oh, you are right! The user does not need to provide Regarding the MongoDB documents, I think we should make it mandatory over there to ensure that all the documents are compliant with a schema. Hence the description in the GH issue asking those documents to be updated. |
Jenkins results:
|
Jenkins results:
|
@amaltaro, I added a new script to make adjustments to MongoDB documents which I named |
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.
Other than one comment left along the code, everything else looks good to me.
Can you please squash those commits in 1 or 2 commits only?
In addition, please clarify either in the script docstring or in the PR description itself on how exactly to run that script. As I understand it, it needs to be executed either from the MongoDB k8s cluster or from a backend pod running in k8s (e.g., MSOutput).
Lastly, please go ahead and update the ms-pileup documents in testbed. Once we are about to converge with these in production, we can then update the production documents as well.
bc349f4
to
9de4e92
Compare
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.
Once testbed documents have been updated, we can get this merged. Thanks
9de4e92
to
0a67508
Compare
Jenkins results:
|
Regarding fixing records on testbed and production, we can't do it until #11765 will be in place on both servers since we rigorously check records and since |
@vkuznet Valentin, now that cmsweb-testbed is running with the relevant customName code, can you please update the MSPileup documents in cmsweb-testbed MongoDB? Thanks |
I modified records in testbed but still haven't had time to perform integration test. |
@vkuznet Thanks Valentin! For the integration tests, perhaps we can wait for other features to get inserted into MSPileup - e.g. automatic creation of new DIDs and block/file attachment. This will make the integration tests easier to be performed. |
Fixes #11737
Status
ready
Description
Add
customName
to adhoc script and to JSON data documents used in pileup creation.Is it backward compatible (if not, which system it affects?)
YES
Related PRs
#11765
External dependencies / deployment changes