Skip to content

add simple disk space check#12

Open
jakubmajercik wants to merge 13 commits intomainfrom
check-resources
Open

add simple disk space check#12
jakubmajercik wants to merge 13 commits intomainfrom
check-resources

Conversation

@jakubmajercik
Copy link

Description

Issue ticket number

Closes #xxxx

Checklist before requesting a review

  • I have performed a self-review of my code

  • Conforms to the Contributing guidelines

  • Proposed changes are described in the CHANGELOG.md

  • I have tested my code with viash ns test --parallel -q <name or namespace>

  • Check the correct box. Does this PR contain:

    • Breaking changes
    • New functionality
    • Major changes
    • Minor changes
    • Documentation
    • Bug fixes

@jakubmajercik jakubmajercik requested a review from tverbeiren June 17, 2025 11:58
Copy link
Contributor

@tverbeiren tverbeiren left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@DriesSchaumont DriesSchaumont left a comment

Choose a reason for hiding this comment

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

Could you add a CHANGELOG entry?
Also, would this not be better suited as a craftbox component?

@jakubmajercik
Copy link
Author

Could you add a CHANGELOG entry? Also, would this not be better suited as a craftbox component?

Isn't craftbox more bioinformatics-oriented while toolbox for more general utilities?

@DriesSchaumont
Copy link
Contributor

Could you add a CHANGELOG entry? Also, would this not be better suited as a craftbox component?

Isn't craftbox more bioinformatics-oriented while toolbox for more general utilities?

Craftbox: craftbox is a curated collection of custom scripts and utilities designed to tackle context-specific tasks.
Its a packages for (1) scripts that we have written ourselfves or (2) functionality where we use some tool, but also change the behavior in some way deviates from the original tool (e.g. the untar component: if the contents of the .tar file is just a single directory, put the contents of the directory into the output folder instead of that directory.)

In toolbox, the goals is to still wrap some tool and mimic the original behavior as close as possible. The behavior is probably not bioinformatics related - this is what biobox is for.

Copy link
Contributor

@rcannood rcannood left a comment

Choose a reason for hiding this comment

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

Can we add sufficient documentation to let people know that this component is only useful when working with persistent storage? That is, if this is being run on aws batch, the code in this component might be run on a totally different instance with different storage than the the next step in the workflow.

Ideally it'd be more useful for this check to be executed as part of a different component.

If we want to add a component like this anyways, it should be added to craftbox, not toolbox. toolbox should only contain cli tools that are wrapped in a straight forward fashion -- similar to biobox but for general (non-bio) purposes.

@jakubmajercik
Copy link
Author

Can we add sufficient documentation to let people know that this component is only useful when working with persistent storage? That is, if this is being run on aws batch, the code in this component might be run on a totally different instance with different storage than the the next step in the workflow.

Ideally it'd be more useful for this check to be executed as part of a different component.

If we want to add a component like this anyways, it should be added to craftbox, not toolbox. toolbox should only contain cli tools that are wrapped in a straight forward fashion -- similar to biobox but for general (non-bio) purposes.

Added some documentation. I will move it to craftbox once this PR is approved.

@jakubmajercik jakubmajercik requested a review from rcannood June 25, 2025 08:36
Copy link
Contributor

@DriesSchaumont DriesSchaumont left a comment

Choose a reason for hiding this comment

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

LGTM! Please move it 👍

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