-
Notifications
You must be signed in to change notification settings - Fork 214
Issues/3466 Move away from Gitpod #3569
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
Conversation
…tools into issues/3466-gitpod
…r. Update postCreateCommand to match context
Note: This still uses (a newer) gitpod base image and thus also the |
Codecov Report✅ All modified and coverable lines are covered by tests. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
"image": "nfcore/gitpod:latest", | ||
"remoteUser": "gitpod", | ||
"runArgs": ["--privileged"], | ||
"image": "nfcore/devcontainer", // ToDo: Add version hash once available |
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 should use the latest one, since this file is updated right before a tools release, we won't have the hash available.
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 is true, renovate bot would always be lacking behind ...
I'll change to latest for now, Updating the version here would make the push workflow a lot more complicated.
tests/pipelines/test_create.py
Outdated
"""Test that all pipeline template files are included in a pipeline customisation group.""" | ||
template_features_yml = load_features_yaml() | ||
base_required_files = [ | ||
".devcontainer/setup.sh", |
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 file is not mandatory, it should be added to the template_features.yml
file (it's already there). Was it added to base_required_files
for a different reason?
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 was one of the tests failing after the template_features.yml
update, which was fixed by updating the snapshots. I think this was left by accident and I'll remove it 👍
Hi, I'm only just back from vacation. What should I test and review at the moment, and what has been tested? |
Welcome back and thanks for having a look @mahesh-panchal This PR changed quite a bit, but this update still sums it up well: #3569 (comment) The main things I could use feedback on:
EDITED after 6c1596c changes |
Review comments have all been addressed and are outdated now
…tools into issues/3466-gitpod
Final changes to address the size of the resulting devcontainerProblem 1: The features added a lot of size (roughly 300MB each. >1GB for the local nextflow feature!) Changes:
Here are some numbers about the Dockerfile (.devcontainer/build-devcontainer/Dockerfile) based on manual builds:
And the resulting devcontainer (.devcontainer/build-devcontainer/devcontainer.json):
|
Good investigation. 4gb still seems a lot too me. But nothing we have to fix in this PR. But maybe make an issue about it. |
I got this error when opening this PR in Codespaces.
and it opened a recovery container instead. |
To test on the nf-core modules repo, which files are the important ones I should transfer to a branch? |
@mahesh-panchal The error is because the image is not yet pushed to Dockerhub. This will work once the first build action (after a pull to main) is run.
The devcontainer EDIT: I built and pushed our base devcontainer image (.devcontainer/build-devcontainer/devcontainer.json) to a registry: fljulian/nfcore-devcontainer:latest. You can use this in EDIT2: You can test this repo in codespaces here: https://github.com/JulianFlesch/nf-core-tools/tree/testing/issue-3266-gitpod based on the image mentioned above |
The files EDIT: Here is a draft PR: nf-core/modules#8983 |
@mashehu on final inspection, the size actually looks really good:
I don't think I locally accounted for layer compression, but dockerhub took care of that for us 👍 |
Description
Addresses the suggestions in issue 3466 to move away from gitpod and towards devcontainer for Github codespaces.
PR checklist
CHANGELOG.md
is updateddocs
is updated