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

Differential tarball tool #9

Merged
merged 20 commits into from Feb 27, 2020
Merged

Conversation

ilanschnell
Copy link

This PR adds the conda-diff-tar command to conda-mirror, a tool for creating tarballs from a previous reference point of a conda repository. The file diff-tar.md explains all the details.

@codecov
Copy link

codecov bot commented Feb 17, 2020

Codecov Report

Merging #9 into master will increase coverage by 1.57%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master       #9      +/-   ##
==========================================
+ Coverage   88.78%   90.36%   +1.57%     
==========================================
  Files           2        3       +1     
  Lines         312      415     +103     
==========================================
+ Hits          277      375      +98     
- Misses         35       40       +5     
Impacted Files Coverage Δ
conda_mirror/diff_tar.py 95.14% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 34280d9...d42cb3f. Read the comment docs.

@ilanschnell
Copy link
Author

Ouch, I should probably add some tests.

conda_mirror/diff_tar.py Outdated Show resolved Hide resolved
conda_mirror/diff_tar.py Outdated Show resolved Hide resolved
conda_mirror/diff_tar.py Outdated Show resolved Hide resolved
@@ -27,7 +27,8 @@
],
entry_points={
"console_scripts": [
'conda-mirror = conda_mirror.conda_mirror:cli'
'conda-mirror = conda_mirror.conda_mirror:cli',
'conda-diff-tar = conda_mirror.diff_tar:main',
Copy link

Choose a reason for hiding this comment

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

Shouldn't this be a subcommand of conda-mirror, rather than a separate command?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure, I realized that just adding the new functionality as options wouldn't make too much sense. Having a sub command it of course possible too, for example conda-mirror diff-tar.

Copy link

Choose a reason for hiding this comment

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

@xhochy @mariusvniekerk - any opinions?

@ilanschnell
Copy link
Author

Thanks for the feedback @scopatz. I've addressed the points. The only remaining question is how the new functionality should be invoked. First, I though that adding options to the existing conda-mirror command would be best, but then I realized that the new functionality is really for a very special (and not very common) case and adding the options to the existing command would probably only be confusing, in particular since most users are never going to need the functionality. This is why I added the conda-diff-tar command. Another option would be to add a sub-command conda-mirror diff-tar, but since conda-mirror doesn't have any sub-commands so far, this would be awkward too. Thoughts?

test/test_diff_tar.py Show resolved Hide resolved
conda_mirror/diff_tar.py Outdated Show resolved Hide resolved
conda_mirror/diff_tar.py Outdated Show resolved Hide resolved
conda_mirror/diff_tar.py Outdated Show resolved Hide resolved
conda_mirror/diff_tar.py Outdated Show resolved Hide resolved
@ilanschnell
Copy link
Author

Thanks @scopatz for suggesting to raise an error and handling it in main. I've made that change, and also added a test for the error being raised in read_reference() when the reference file is not present. I hope we are ready to merge this now.

@xhochy xhochy merged commit 202355f into conda-incubator:master Feb 27, 2020
@xhochy
Copy link
Member

xhochy commented Feb 27, 2020

Thanks @ilanschnell !

@scopatz
Copy link

scopatz commented Feb 27, 2020

I am going to cut a release with this capability

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.

None yet

3 participants