DM-33619: Add unlink option to butler remove-runs #661
Conversation
n8pease
force-pushed
the
tickets/DM-33619
branch
5 times, most recently
from
6936dbe
to
6e27018
Compare
Codecov Report
@@ Coverage Diff @@
## main #661 +/- ##
==========================================
+ Coverage 83.69% 83.72% +0.03%
==========================================
Files 239 239
Lines 30457 30528 +71
Branches 5110 5120 +10
==========================================
+ Hits 25492 25561 +69
Misses 3811 3811
- Partials 1154 1156 +2
Continue to review full report at Codecov.
|
| # and exit. | ||
| if any(run.parents for run in result.runs) and not force: | ||
| _print_requires_confirmation(result.runs, result.datasets) | ||
| sys.exit(1) |
There was a problem hiding this comment.
Is Click okay with calling sys.exit()? I see there is a click.Context.exit().
| @@ -196,6 +196,9 @@ def addArgumentHelp(doc, helpText): | |||
| # (Lines after the first blank line will be argument help.) | |||
| while len(doclines) > 1 and doclines[1]: | |||
| doclines[0] = " ".join((doclines[0], doclines.pop(1).strip())) | |||
| # Add standard indent to help text for proper alignment with command | |||
| # function documentation: | |||
| helpText = " " + helpText | |||
There was a problem hiding this comment.
Word wrapping in click does seem harder than it should be...
There was a problem hiding this comment.
This is partially my fault, for modifying how click.argument help is inserted; click says you should add it yourself in the command function doctoring, but I wanted to reuse the help text along with the argument declaration via the argument decorator. I'm kind of fighting the API. If it's a continuous PITA then probably I should back out what I've done & let the wookie win.
| """Perform the remove step.""" | ||
| butler = Butler(repo, writeable=True) | ||
| butler.removeRuns(runs, unstore=True) | ||
| for run in runs: |
There was a problem hiding this comment.
Should this entire loop be in a transaction in case something goes wrong before the removeRuns happens and you may not want some of the chains to have been modified and some unchanged?
There was a problem hiding this comment.
that seems like a good idea. I think to do that I put the loop inside a block that begins with with butler.transaction():, and that's all, is that right?
tests/test_cliCmdRemoveRuns.py
Outdated
| @@ -83,8 +95,25 @@ def test_removeRuns(self): | |||
| root = "repo1" | |||
| MetricTestRepo(root, configFile=os.path.join(TESTDIR, "config/basic/butler.yaml")) | |||
|
|
|||
| # Execute cmd but say no, check for expected outputs. | |||
| # add the run to a CHAINED collection | |||
There was a problem hiding this comment.
As a general point comments should be sentences so this should be:
# Add the run to a CHAINED collection.| if run.parents: | ||
| print(unlinkMsg.format(run=run.name, parents=", ".join(_quoted(run.parents)))) | ||
| else: | ||
| print(run.name) |
There was a problem hiding this comment.
Does the coverage report indicating that this line never runs mean that we aren't testing the command without parents?
There was a problem hiding this comment.
yes, I changed the tests for CHAINED collections, and didn't consider this case. Will fix.
| # Execute cmd with --force | ||
| result = self.runner.invoke( | ||
| butler.cli, ["remove-runs", root, "ingest*", "--no-confirm", "--force"] | ||
| ) |
There was a problem hiding this comment.
Can we also run butler query-collections for the run to show it has disappeared?
When inserting argument help, it needs to have the same indent as the help text around it. This adds the standard 4 spaces.
n8pease
force-pushed
the
tickets/DM-33619
branch
from
6e27018
to
3f61768
Compare
Checklist
doc/changes