Skip to content
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

Added changes for user credentials #392

Merged
merged 35 commits into from May 4, 2021

Conversation

WenzDaniel
Copy link
Contributor

@WenzDaniel WenzDaniel commented Mar 4, 2021

What is the problem / what does the code in this PR do
In this PR we add the requirement of putting personalized user credentials before queering things from our Slow Control Historian database. The syntax has not changed much as shown in the the very last section of this notebook.

For some of our automatized services such as the online monitor and CMT we would need an additional "strax user account" which we can store in our .xenon_config file. In order to use this additional user the corresponding tools have to set a flag which is "private" to the interface and not visible from the outside.

Beside asking for the user credentials I added some quality of life features like a function to ask for the remaining time a token is valid as well as two safeguards to ensure that a query does not fail because of an expired token. In the later case a renewing of the credentials is necessary.

Further, I had to reduce the complexity of get_values. Hence, I clustered all time related checks and computations in a small helper function.

@WenzDaniel WenzDaniel marked this pull request as draft March 4, 2021 08:26
@WenzDaniel WenzDaniel marked this pull request as ready for review March 4, 2021 12:37
@JoranAngevaare JoranAngevaare removed their request for review March 9, 2021 20:02
@JoranAngevaare
Copy link
Contributor

Hi Daniel, it is indeed perhaps a good idea if some SC people look into this too. Perhaps ACs can help out if more eyes are needed.

@kdund
Copy link
Member

kdund commented Mar 9, 2021

terminology: "strax user account" refers to an account for strax users of the SC historians?

straxen/scada.py Show resolved Hide resolved
@WenzDaniel
Copy link
Contributor Author

Hej, thanks for the comments. This PR triggered some discussions if we should not go for a different authentication approach. Hence, it might be that the changes will never come. I will change the PR into a draft should have done this before, I am sorry about that.

terminology: "strax user account" refers to an account for strax users of the SC historians?
It refers to a SC Historian account for strax. Some of our services as the online monitor and CMT need an account which do not require to manually reenter the credentials every 3 hrs.

@WenzDaniel WenzDaniel marked this pull request as draft March 10, 2021 06:43
@WenzDaniel WenzDaniel marked this pull request as ready for review March 30, 2021 06:36
@WenzDaniel
Copy link
Contributor Author

The failed test points to the test of the holoviews waveform plot. Dask dataframes are missing during import. I do not know why it is showing up now since the test was already added some time ago.

@WenzDaniel
Copy link
Contributor Author

So this PR is ready for merge, but we should wait until the next release since we have to updated the xenon config too.

Copy link

@hagarlandsman hagarlandsman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raise ValueError(f'Cannot load SCADA information, from your xenon'
' config. SCADAInterface cannot be used.') from e

straxen/scada.py Outdated Show resolved Hide resolved
straxen/scada.py Outdated Show resolved Hide resolved
straxen/scada.py Outdated Show resolved Hide resolved
straxen/scada.py Show resolved Hide resolved
straxen/scada.py Outdated Show resolved Hide resolved
straxen/scada.py Show resolved Hide resolved
straxen/scada.py Show resolved Hide resolved
straxen/scada.py Outdated Show resolved Hide resolved
straxen/scada.py Show resolved Hide resolved
straxen/scada.py Show resolved Hide resolved
@WenzDaniel
Copy link
Contributor Author

Thanks for the thorough review. I addressed your comments. We can merge this PR with the next release, have to time it a little bit though since we have to updated the xenon_config.

@JoranAngevaare JoranAngevaare added the enhancement New feature or request label May 3, 2021
@WenzDaniel WenzDaniel merged commit dce368c into XENONnT:master May 4, 2021
JoranAngevaare added a commit that referenced this pull request May 4, 2021
* allow scada

* remove key

* get brackets right for sectres
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants