-
Notifications
You must be signed in to change notification settings - Fork 4.4k
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
Change DBS url to cmsweb-prod in MatrixInjector #33209
Conversation
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33209/21646
|
A new Pull Request was created by @justinasr (Justinas Rumševičius) for master. It involves the following packages: Configuration/PyReleaseValidation @jordan-martins, @chayanit, @wajidalikhan, @kpedro88, @cmsbuild, @srimanob can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
please test |
@@ -71,7 +71,7 @@ def __init__(self,opt,mode='init',options=''): | |||
if not self.wmagent: | |||
if not opt.testbed : | |||
self.wmagent = 'cmsweb.cern.ch' | |||
self.DbsUrl = "https://"+self.wmagent+"/dbs/prod/global/DBSReader" | |||
self.DbsUrl = "https://cmsweb-prod.cern.ch/dbs/prod/global/DBSReader" |
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.
although it was already hardcoded, why not configure it via env variable. This will help in future when we have to change it again.
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.
That would be the most flexible way, however it would then depend on each user having to export
it before every use. Unless you are talking about env variables that are set by cmsenv
, then yes, it could be set there. I also don't expect this to change that often that it would cause too much of inconvenience to change the hardcoded value.
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.
@justinasr , I mean use the env variable to override the default. If env variable is set then we use it otherwise fallback to default ( hardcoded in the release).
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.
@smuzaffar sure, should the whole url be configurable or just the host? Would these be appropriate names for the env variables: DBSREADER_URL
(if the whole url is configurable) or DBSREADER_HOST
(if only host is configurable)?
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.
@justinasr , I would suggest to make whole url configurable. About the name, I would suggest to use CMS_DBSREADER_URL
. If you want, you can also add command-line option e.g. --dbs-url ful-dbs-url
to runTheMatrix.py
:-)
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f3e720/13589/summary.html Comparison SummarySummary:
|
Hi @smuzaffar @justinasr |
@srimanob , as we are changing it from one hardcoded value to another, I suggested to provide a way to override it in future via an environment variable ( see #33209 (comment) ) . |
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-33209/21806
|
Pull request #33209 was updated. @jordan-martins, @chayanit, @wajidalikhan, @kpedro88, @cmsbuild, @srimanob can you please check and sign again. |
@smuzaffar Updated so it would be configurable with env variable and |
Please test |
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-f3e720/13827/summary.html Comparison SummaryThe workflows 140.53 have different files in step1_dasquery.log than the ones found in the baseline. You may want to check and retrigger the tests if necessary. You can check it in the "files" directory in the results of the comparisons Summary:
|
+Upgrade This is a technical PR to assign dbsUrl attribute in the dictionary, which is submitted to ReqMgr2 by MatrixInjector. @smuzaffar This update looks OK for me. Is it as you expect? Thanks. |
yes @srimanob this looks good now. |
+1 |
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2) |
+1 |
PR description:
Use cmsweb-prod.cern.ch for "DbsUrl" attribute in the dictionary that is submitted to ReqMgr2 by MatrixInjector.
Relevant ticket on gitlab: https://gitlab.cern.ch/cms-http-group/doc/-/issues/256
PR validation:
None.
If this PR is a backport please specify the original PR and why you need to backport that PR:
Not a backport.