-
Notifications
You must be signed in to change notification settings - Fork 156
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
Local devcontainers enablement #539
base: master
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for nextflow-training ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
How does this work when creating a codespace? I'm not well read on the default file paths and similar. Does Codespaces pick the correct one by default? How does local devcontainer setup know which one to use? |
I think Codespaces will use the one at the root of .devcontainers by default (which is why I added that symlink). The VSCode plugin asks you to choose, so we just pick the local config when it does. |
Updated diff: 2c2
< "name": "nextflow-training (codespaces)",
---
> "name": "nextflow-training (local)",
14c14,15
< "workspaceFolder": "/workspaces/training",
---
> "workspaceFolder": "${localWorkspaceFolder}",
> "workspaceMount": "source=${localWorkspaceFolder},target=${localWorkspaceFolder},type=bind",
16c17
< "NXF_HOME": "/workspaces/.nextflow",
---
> "NXF_HOME": "/workspaces/training/.nextflow",
18a20
> "postCreateCommand": "sudo mkdir -p /workspaces && sudo rm -rf /workspaces/training && sudo ln -s ${localWorkspaceFolder} /workspaces/training",
22c24,26
< "python.defaultInterpreterPath": "/opt/conda/bin/python"
---
> "python.defaultInterpreterPath": "/opt/conda/bin/python",
> "terminal.integrated.cwd": "/workspaces/training",
> "terminal.integrated.defaultLocation": "workspace"
|
ok yup, agree that this seems to be the case. I think we could probably pre-set this in the codespaces link that we have in the training, but if that link isn't used then it sounds like a very standard one would be set by default, so safer to symlink it as you did. All good 👍🏻 |
"nextflow.nextflow", // Nextflow VS Code extension | ||
"codezombiech.gitignore", // Language support for .gitignore files | ||
"cssho.vscode-svgviewer" // SVG viewer |
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.
I think we can simplify these - nextflow.nextflow
shouldn't be needed as it's already part of the nf-core extension pack, and the other two could be added to that. Then we just have the nf-core extension pack and be done with it.
I'd also like to remove a couple of the more subjective packages from the nf-core extension pack (rainbow indentation, TODO tree, code spell checker). I find myself disabling these each time in devcontainers, which is annoying.
We can do this in a separate PR, as needs upstream changes to the nf-core extension pack first.
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.
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 SVG viewer extension is archived and no longer present in the marketplace, this can be removed.
@pinin4fjords we should update the orientation docs to match this change, ideally with a screenshot etc. |
@ewels I was kind of thinking of not advertising until we needed it, but I guess docs can't hurt. In the process of verifying docs, I discovered that my changes weren't compatible with your latest tidying up. The devcontainers plugin on my Mac insisted on being an ARM64 whatever I tried, and the devcontainers base image you used is AMD64 only. I couldn't get the pre-built image to work with the docker-outside-docker either, which is probably related. I switched the image to one that's cross-compatible, and made the local devcontainer revert to building each time. Not sure if, if we update |
@pinin4fjords I'd rather just build the image in multiple formats. It's already using buildx, so it shouldn't be difficult to make it multi-arch. |
"build": { | ||
"dockerfile": "../Dockerfile", | ||
"context": "..", | ||
"options": ["--platform=linux/amd64"] |
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.
Do we really want to force people to use amd64? Is this needed?
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.
It actually was. I got some weird Docker shim error without 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 maybe if you build the arm image it won't be needed
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.
Two devcontainers files, one for Codespaces, one for local. The codespaces one is unchanged from master, I've just moved it to the subdir.
non-codespaces ('local') operation involves mirroring paths from host, and symlinking to create the /workspaces location for harmony with the materials. We then set all terminals to open in that path.
Symlinked the codespaces version at the root so it's the default.
Local operation would be a little more clunky, but would now work. I've checked that the DoD functionality is restored.