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

[ENH] Better devops and cleanup #95

Closed
wants to merge 14 commits into from
Closed

Conversation

anibalsolon
Copy link
Member

@anibalsolon anibalsolon commented Sep 27, 2023

This PR attempts to address some issues with the current repo organization and development flow:

  • Environment configuration information is now contained in .env files, which are better for managing: configuration should not be code, tons of infrastructure rely on key-value files, and it is more readable/reliable
  • The configuration js file could be easily overwritten in production, so now the separation using .env.dev prevents this
  • The package management was confusing, there was a package.json in root, then one in /ui, etc so now each part of the system (api, ui, handler) have their own Dockerfile and package.json
  • There were transpiled files (.js) that should not be there, they are generated automatically
  • Files uploaded to ezBIDS would have root permission, and if the developer need to use sudo to do something, it's straight wrong. Now, it uses host user ID to run docker, and the data has the correct permissions

⚠️ it requires to stop ezbids containers and rebuild the images
you can stop and rebuild the images using:
./dev.sh stop and ./dev.sh build

Copy link
Collaborator

@bhatiadheeraj bhatiadheeraj left a comment

Choose a reason for hiding this comment

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

Looks great !

Comment on lines 76 to 67
RUN tar -xzf /ROBEXv12.linux64.tar.gz
RUN mkdir /ROBEX && \
curl -SL 'https://www.nitrc.org/frs/download.php/5994/ROBEXv12.linux64.tar.gz//?i_agree=1&download_now=1' | \
tar -xvzC /ROBEX
Copy link
Collaborator

Choose a reason for hiding this comment

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

@anibalsolon, this is causing an error when trying to use quickshear for anatomical defacing.

failed to run deface -- code:1
+ defaced=./quick_nifti_test/time-20200122142947-sn-2-name-anat-T1w.nii.gz.defaced.nii.gz
+ echo '--------------- defacing(quickshear) [0] ./quick_nifti_test/time-20200122142947-sn-2-name-anat-T1w.nii.gz to ./quick_nifti_test/time-20200122142947-sn-2-name-anat-T1w.nii.gz.defaced.nii.gz ----------------'
+ case $method in
+ runROBEX.sh ./quick_nifti_test/time-20200122142947-sn-2-name-anat-T1w.nii.gz ./quick_nifti_test/time-20200122142947-sn-2-name-anat-T1w.nii.gz.mask.nii.gz
environment: line 10: runROBEX.sh: command not found

real	0m0.000s
user	0m0.000s
sys	0m0.000s

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for testing, @dlevitas ! It should be pointing now to the correct file.

dlevitas
dlevitas approved these changes Oct 4, 2023
@nicoalee
Copy link
Collaborator

nicoalee commented Oct 4, 2023

@anibalsolon handler/bids.sh currently imports ./convert.js. This needs to be changed to convert.ts
image

Copy link
Collaborator

@nicoalee nicoalee left a comment

Choose a reason for hiding this comment

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

Some changes need to be addressed as currently:
(1) (IMPORTANT) the finalize step is broken
- convert.js was changed to convert.ts. bids.sh still imports convert.js
- handler/package.json needs to include async in its dependencies
(2) (IMPORTANT) the swagger stuff is broken
(3) (minor) stylistic improvements and consistency with dev files

Addressing my comments should solve these issues


//import sendSeekable = require('send-seekable');
import * as models from "./models";
import controllers from "./controllers";
Copy link
Collaborator

@nicoalee nicoalee Oct 4, 2023

Choose a reason for hiding this comment

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

if we import controllers like this, then we should export it as default in the controllers.ts file. Instead of module.exports = router it should be export default router

api/ezbids.ts Outdated Show resolved Hide resolved
api/ezbids.ts Show resolved Hide resolved
api/ezbids.ts Outdated Show resolved Hide resolved
api/ezbids.ts Outdated Show resolved Hide resolved
api/ezbids.ts Outdated Show resolved Hide resolved
handler/handler.ts Show resolved Hide resolved
"author": "",
"license": "MIT",
"dependencies": {
"dotenv": "^16.3.1",
Copy link
Collaborator

Choose a reason for hiding this comment

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

include async in dependencies as required by convert.ts

api/package.json Outdated
"deploy": "ssh -t [email protected] \"sudo su - -c 'cd /root/docker/ezbids && ./update.sh'\"",
"deploy-prod": "ssh -t -J [email protected] ubuntu@ezbids \"sudo su - -c 'cd /root/docker/ezbids && ./update.sh'\""
"dev": "ts-node-dev --respawn --transpile-only ezbids.ts",
"prod": "tsc && node ./build/ezbids.js"
Copy link
Collaborator

Choose a reason for hiding this comment

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

are files transpiled into JS when built? Just double checking that the prod script here was intentional and shouldn't instead be: "prod": "tsc && node ./build/ezbids.ts"

"description": "",
"scripts": {
"dev": "ts-node-dev --respawn --transpile-only handler.ts",
"prod": "tsc && node ./build/handler.js"
Copy link
Collaborator

Choose a reason for hiding this comment

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

are files transpiled into JS when built? Just double checking that the prod script here was intentional and shouldn't instead be: "prod": "tsc && node ./build/handler.ts"

@anibalsolon anibalsolon marked this pull request as draft November 15, 2023 19:53
@anibalsolon anibalsolon closed this Nov 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants