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

Add --assume-ready for image and extra-inputs #205

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

bpinsard
Copy link

@bpinsard bpinsard commented Mar 14, 2023

Add --assume-ready option to be passed to run, with extra options for image and extra-inputs.
see #199 .

TODOs

  • add basic testing for added option/functionality

@codeclimate
Copy link

codeclimate bot commented Mar 14, 2023

Code Climate has analyzed commit 380ead9 and detected 0 issues on this pull request.

View more on Code Climate.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

just initial peek.

datalad_container/containers_run.py Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Mar 15, 2023

Codecov Report

Attention: Patch coverage is 92.85714% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 94.80%. Comparing base (d5f01ae) to head (380ead9).

Files Patch % Lines
datalad_container/containers_run.py 85.71% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #205      +/-   ##
==========================================
+ Coverage   94.64%   94.80%   +0.16%     
==========================================
  Files          24       24              
  Lines        1120     1155      +35     
==========================================
+ Hits         1060     1095      +35     
  Misses         60       60              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@yarikoptic yarikoptic left a comment

Choose a reason for hiding this comment

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

Left some minor recommendations but overall I think it should be ok.
But now it would need some basic test added since code with tests is ... broken code ;) See e.g. recently added tests for extra_inputs handling in https://github.com/datalad/datalad-container/blob/HEAD/datalad_container/tests/test_run.py#L195

datalad_container/containers_run.py Outdated Show resolved Hide resolved
datalad_container/containers_run.py Outdated Show resolved Hide resolved
datalad_container/containers_run.py Outdated Show resolved Hide resolved
datalad_container/containers_run.py Outdated Show resolved Hide resolved
@yarikoptic
Copy link
Member

I will move it to Draft. Please bring it back to "Ready for Review" whenever it gets extended with a test @bpinsard .

@yarikoptic yarikoptic marked this pull request as draft May 25, 2023 16:33
@yarikoptic
Copy link
Member

ping @bpinsard any time to finish this one up?

@bpinsard bpinsard marked this pull request as ready for review March 19, 2024 14:59
@yarikoptic yarikoptic changed the title add assume ready for image (and extra-inputs) Add --assume-ready for image and extra-inputs Mar 20, 2024
@yarikoptic yarikoptic added minor Increment the minor version when merged CHANGELOG-missing labels Mar 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Increment the minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants