36 extend workflowoutput class for sampletype handling - #37
Conversation
molversmyr
left a comment
There was a problem hiding this comment.
Outstanding work!👏🏼👏🏼
I've added a few comments, most of which are suggested optional modifications. Feel free to add or ignore them as you wish, I'll approve the PR now anyway 😄
As I mentioned in one general file comment, I didn't scrutinise all the test cases manually, but went through some of them to get a better understand the src/tsoppy/general/classes.py and src/tsoppy/general/file_parser.py code.
Also very nice for me as well to get some test data to understand the structure of the input and output files for both LocalApp and Dragen (at least for the version that are in production). So thank you very much for that! 🙌🏼
| "dockerfile": "Dockerfile", | ||
| "args": { | ||
| "USERNAME": "${localEnv:USER}", | ||
| "USER_UID": "${localEnv:UID}", |
There was a problem hiding this comment.
Don't exactly know why, but I need to hard-code/set both "USER_UID" and "USER_GID" to specific values for my MacBook to be able to build the container. I've set both of them to 1001 (GID 1000 already exists, otherwise arbitrarily chosen number).
The error occurs in .devcontainer/Dockerfile (line 27 the grouppadd function). The error occurs seemingly because groupadd is trying to add my personal USERNAME 'havard' as the group ID, but I don't know why or how to properly solve this for universal OS devcontainer use.
| "args": { | ||
| "USERNAME": "${localEnv:USER}", | ||
| "USER_UID": "${localEnv:UID}", | ||
| "USER_GID": "${localEnv:UID}", |
| }, | ||
| "mounts": [ | ||
| // Mount in forwarding of ssh keys | ||
| "source=${localEnv:SSH_AUTH_SOCK},target=${localEnv:SSH_AUTH_SOCK},type=bind" |
There was a problem hiding this comment.
I also need to comment out this mount as my local SSH_AUTH_SOCK (default – not personally set by me) is not accessible for the devcontainer.
Co-authored-by: Håvard Molversmyr <54852797+molversmyr@users.noreply.github.com>
Cool, thank you for the fast review and really good feedback, I have applied all of your suggestions! Seems like you have an eye for these details 👀 I am not sure what to do about the .devcontainer part as I cannot test it on my side (we are using a remote linux server for development). I remember that @tinavisnovska also had troubles with the devcontainer so it might be good to find a way to support both OS somehow. I need to check if I can find a solution online. Will update it accordingly. I might just merge this now and create an issue to keep the concerns of the PR a bit separated. Thanks again @molversmyr 🙏 |
WorkflowOutputclass parses the samplesheet now which allows to check for existing samplesNonetype