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

Space check #70

Merged
merged 9 commits into from Oct 26, 2018

Conversation

@magnuhho
Copy link
Contributor

magnuhho commented Sep 24, 2018

By default conda-mirror will continue pulling files until it either succeeds or dies from some sort of error.
This change aborts downloads with a logger.error when a threshold is reached.

@magnuhho

This comment has been minimized.

Copy link
Contributor Author

magnuhho commented Sep 24, 2018

Whoops. Didn't see the work done and the comments to #64.

@parente

This comment has been minimized.

Copy link
Collaborator

parente commented Sep 25, 2018

@magnuhho Thanks for the series of PRs. I need to get travis working again properly for the repo (it changed orgs) and will give them a look-see.

@parente parente closed this Sep 25, 2018
@parente parente reopened this Sep 25, 2018
@parente parente closed this Sep 25, 2018
@parente parente reopened this Sep 25, 2018
@@ -26,6 +26,7 @@
'win-64',
'win-32']

MINIMUM_FREE_SPACE_MB = 1000

This comment has been minimized.

Copy link
@parente

parente Sep 27, 2018

Collaborator

It would help to make variable a command line argument instead so that users can decide what minimum means on their infrastructure and testing becomes more feasible.

This comment has been minimized.

Copy link
@magnuhho

magnuhho Oct 15, 2018

Author Contributor

parente, apologies for the late reply.
A command line argument is a good idea, but "instead of" or "in addition to".
Don't you think a built-in default would be good so default behavior is to stop before the disks are full?

This comment has been minimized.

Copy link
@parente

parente Oct 24, 2018

Collaborator

A command line parameter with that default if not specified sounds good to me.

@@ -678,6 +679,10 @@ def main(upstream_channel, target_directory, temp_directory, platform,
with tempfile.TemporaryDirectory(dir=temp_directory) as download_dir:
logger.info('downloading to the tempdir %s', download_dir)
for package_name in sorted(to_mirror):
disk = os.statvfs(download_dir)

This comment has been minimized.

Copy link
@parente

parente Sep 27, 2018

Collaborator

https://docs.python.org/3/library/os.html#os.fstatvfs suggests this function is Unix-only. Are you aware of a cross-platform solution for Windows users (which I think we have)?

This comment has been minimized.

Copy link
@magnuhho

magnuhho Oct 15, 2018

Author Contributor

We can use shutil.disk_usage if we can assume Python 3.3 or newer?
https://docs.python.org/3/library/shutil.html#shutil.disk_usage

This comment has been minimized.

Copy link
@parente

parente Oct 24, 2018

Collaborator

We only test for 3.4+ and up (https://github.com/Valassis-Digital-Media/conda-mirror/blob/master/.travis.yml) and declare it's a Python 3 only package (https://github.com/Valassis-Digital-Media/conda-mirror#compatibility) so I think shutil.disk_usage sounds perfect.

@magnuhho

This comment has been minimized.

Copy link
Contributor Author

magnuhho commented Oct 24, 2018

added changes to

  • use command line argument instead of constant
  • make the space check windows compatible
  • check both temp and target folders
  • take already downloaded packages into account when checking free space in target folder
magnuhho added 3 commits Oct 25, 2018
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 25, 2018

Codecov Report

Merging #70 into master will decrease coverage by 1.19%.
The diff coverage is 64.28%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master     #70     +/-   ##
========================================
- Coverage   93.89%   92.7%   -1.2%     
========================================
  Files           2       2             
  Lines         262     274     +12     
========================================
+ Hits          246     254      +8     
- Misses         16      20      +4
Impacted Files Coverage Δ
conda_mirror/conda_mirror.py 92.61% <64.28%> (-1.21%) ⬇️

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 96bad91...a29a55d. Read the comment docs.

@parente

This comment has been minimized.

Copy link
Collaborator

parente commented Oct 26, 2018

Thanks for working through this @magnuhho. It looks reasonable now. We'll probably want to add a test for it in the future, but let's get this merged for now. 🎉

@parente parente merged commit 4233ea6 into Valassis-Digital-Media:master Oct 26, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@parente parente mentioned this pull request Jan 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.