-
Notifications
You must be signed in to change notification settings - Fork 2
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
Exporter, Teiler, QB, DataSHIELD #164
Conversation
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 resolve the 2 todo's and then we car merge it
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.
One thing that might cause a problem when bridgeheads update is that mtba now uses secret sync which requires the new /var/cache/bridgehead dir to be present which is only created after a new bridgehead install. I dont think this will be a huge problem as Pierre said mtba was disable everywhere but just in case.
ccp/modules/datashield-setup.sh
Outdated
if [ "$ENABLE_DATASHIELD" == true ]; then | ||
# HACK: This only works because exporter-setup.sh and teiler-setup.sh are sourced after datashield-setup.sh | ||
ENABLE_EXPORTER=true | ||
ENABLE_TEILER=true |
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.
Not hard dependency please remove it
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.
BUT please give a warning log!
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.
Done
bbmri/modules/dnpm-compose.yml
Outdated
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 will conflict, as the bbmri modules for dnpm are removed in main.
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.
Should we remove this file?
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.
Removed
ccp/modules/datashield-setup.sh
Outdated
if [ "$ENABLE_DATASHIELD" == true ]; then | ||
# HACK: This only works because exporter-setup.sh and teiler-setup.sh are sourced after datashield-setup.sh | ||
ENABLE_EXPORTER=true | ||
ENABLE_TEILER=true |
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.
BUT please give a warning log!
ccp/modules/datashield-sites.json
Outdated
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.
What is this used for? Having those sites hardoced is not very elegant, esp. when it comes to change, update and restart management
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.
These are used to generate the mappings for beam-connect we might want to host them centrally at some point but for now this is really convenient since when we update them the bks are gonna restart automatically.
ccp/modules/datashield.md
Outdated
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.
OK for now, but needs further details
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.
ccp/modules/teiler-setup.sh
Outdated
DEFAULT_LANGUAGE=DE | ||
DEFAULT_LANGUAGE_LOWER_CASE=${DEFAULT_LANGUAGE,,} |
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.
The ENV-Name is quite general. Maybe TEILER_DEFAULT_LANGUAGE*
is more appropriate
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.
While presently utilized exclusively by the "teiler," it's advisable to provide bridgehead developers with a global variable for default language to ensure consistency whenever internationalization is employed.
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.
Co-authored-by: Tobias Kussel <[email protected]>
Co-authored-by: Jan <[email protected]>
New pull request based on old #72. This PR includes the following features:
A summary of the changes made after the review in PR 72 can be found here: https://wiki.verbis.dkfz.de/pages/viewpage.action?pageId=269910138