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

Handle fully qualified channels #25

Merged

Conversation

@ericdill
Copy link
Contributor

ericdill commented Feb 4, 2017

Can you give this branch a try @dmarkwat? I haven't finished fixing up the test suite to allow fully qualified channels like this, but I'm pretty sure it's functional. This should enable you to pass fully qualified channels like

conda-mirror --upstream-channel https://repo.continuum.io/pkgs/free --target-directory /tmp/free --platform linux-64

or the pro one, in your case.

(I think) you can install this with pip install git+https://github.com/ericdill/conda-mirror@issue25/conda-mirror. Will make sure after I open the PR and update this if need be

@ericdill

This comment has been minimized.

Copy link
Contributor Author

ericdill commented Feb 4, 2017

Alright here's the full super-duper magic git install command that will pip install this from the branch on my fork:

pip install -e git://github.com/ericdill/conda-mirror@handle-fully-qualified-channels#egg=conda-mirror
@dmarkwat

This comment has been minimized.

Copy link

dmarkwat commented Feb 5, 2017

Just gave this a try. Everything was working well until it got past the validation of the downloaded packages and yielded this in the output:

INFO: download_url=https://conda.anaconda.org/pro/linux-64/repodata.json
DEBUG: downloading to /tmp/anaconda-pro/tmplpcj2olp/repodata.json
INFO: Not validating /tmp/anaconda-pro/tmplpcj2olp/repodata.json because validate is False and package_metadata is None

Downloading that repodata.json file (which has an empty packages key and doesn't match the repodata.json file found under https://repo.continuum.io/pkgs/prolinux-64/repodata.json) overwrote the good repodata.json file from the continuum.io site. So all the packages were downloaded and validated as expected, but the metadata is getting lost. (Also, not sure why those repodata.json files don't match or why the one from conda.anaconda.org would have an empty packages key...?)

Maybe instead of using the REPODATA url the DOWNLOAD_URL var could just have repodata.json appended to the end? From what I understand, repodata.json is always hosted in the channel's arch directory, no?

@dmarkwat

This comment has been minimized.

Copy link

dmarkwat commented Feb 5, 2017

Awesome commenting by the way - was easy to find the line that was causing this. Not sure if my suggestion would work 100% of the time, though, given the uses of REPODATA. I plan to look closer on Monday if you don't get a chance to comment before then.

@ericdill

This comment has been minimized.

Copy link
Contributor Author

ericdill commented Feb 5, 2017

Awesome commenting by the way

thanks :)

INFO: download_url=https://conda.anaconda.org/pro/linux-64/repodata.json

That's the totally wrong url 😞

the DOWNLOAD_URL var could just have repodata.json appended to the end?

Yup. That's a good idea. I'll push up a change shortly and ping you again

From what I understand, repodata.json is always hosted in the channel's arch directory, no?

That is my understanding as well

ericdill added 3 commits Feb 5, 2017
Also update the docs and general clean up
- Fake a noarch channel since conda appears to need that now
@ericdill

This comment has been minimized.

Copy link
Contributor Author

ericdill commented Feb 5, 2017

Alright turns out that was bit more work than expected. conda-mirror should now handle your use case @dmarkwat . Can you give this branch a shot again?

@dmarkwat

This comment has been minimized.

Copy link

dmarkwat commented Feb 6, 2017

Just tried it out and it works great on the pro repo. I'll probably switch free over to using the URL approach, too. I did find one issue, but it was so small a PR seemed excessive :)

diff --git a/conda_mirror/conda_mirror.py b/conda_mirror/conda_mirror.py
index ee9861c..9083c69 100644
--- a/conda_mirror/conda_mirror.py
+++ b/conda_mirror/conda_mirror.py
@@ -153,6 +153,12 @@ def _make_arg_parser():
         help="Enable PDB debugging on exception",
         default=False,
     )
+    ap.add_argument(
+        '--version',
+        action="store_true",
+        help="Print version and quit",
+        default=False,
+    )
     return ap
@ericdill

This comment has been minimized.

Copy link
Contributor Author

ericdill commented Feb 6, 2017

Just tried it out and it works great on the pro repo. I'll probably switch free over to using the URL approach, too

Excellent. Thanks for being a beta tester 😁

re: --version flag, thanks!

I'll update the tests today, merge this and cut a release

@dmarkwat

This comment has been minimized.

Copy link

dmarkwat commented Feb 6, 2017

Awesome! And just in time for me to setup my crontabs :) Thanks!

@ericdill

This comment has been minimized.

Copy link
Contributor Author

ericdill commented Feb 6, 2017

@dmarkwat Hah so turns out it was actually very simple to fix up the test suite. I was honestly expecting it to be very broken. Any more comments on this PR?

@codecov

This comment has been minimized.

Copy link

codecov bot commented Feb 6, 2017

Codecov Report

Merging #25 into master will not impact coverage by -2.47%.

@@            Coverage Diff             @@
##           master      #25      +/-   ##
==========================================
- Coverage   92.59%   90.12%   -2.47%     
==========================================
  Files           2        2              
  Lines         216      243      +27     
==========================================
+ Hits          200      219      +19     
- Misses         16       24       +8
Impacted Files Coverage Δ
conda_mirror/conda_mirror.py 90% <80.43%> (-2.49%)

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 e8d93c8...32ed8d9. Read the comment docs.

url = default_url_base + url_suffix
return url, channel
# looks like we are being given a fully qualified channel
download_base, channel = channel.rsplit('/', 1)

This comment has been minimized.

Copy link
@parente

parente Feb 6, 2017

Collaborator

Would using any of the urllib.parse functions like urljoin make this more robust?

This comment has been minimized.

Copy link
@parente

parente Feb 6, 2017

Collaborator

"This" meaning the code in this function.

This comment has been minimized.

Copy link
@ericdill

ericdill Feb 6, 2017

Author Contributor

Yes perhaps. I have not used any of that before

@@ -273,29 +355,56 @@ def _download(url, target_directory, package_metadata=None, validate=True,


def _list_conda_packages(local_dir):
"""List the conda packages (*.tar.bz2 files) in `local_dir

This comment has been minimized.

Copy link
@parente

parente Feb 6, 2017

Collaborator

Missing `.

"""Validate local conda packages.
NOTE: This is slow.
NOTE2: This will remove any packages that are in `package_directory` that

This comment has been minimized.

Copy link
@parente

parente Feb 6, 2017

Collaborator

It will also remove any packages that fail validation because that's what _validate does. Maybe worth mentioning?

This comment has been minimized.

Copy link
@ericdill

ericdill Feb 7, 2017

Author Contributor

updated

@dmarkwat

This comment has been minimized.

Copy link

dmarkwat commented Feb 7, 2017

No extra comments from me; all is working as expected from what I can tell. Thanks for the help!

ericdill added 7 commits Feb 7, 2017
no flag: ERROR only
-v: WARNING + ERROR
-vv: INFO + WARNING + ERROR
-vvv: DEBUG + INFO + WARNING + ERROR
@@ -262,6 +262,10 @@ def _get_output(cmd):
except subprocess.CalledProcessError as cpe:
logger.exception(cpe.output.decode())
return ""
except Exception:
msg = "Error in subprocess.check_output. cmd: '%s'"
logger.exception(msg, ' '.join(cmd))

This comment has been minimized.

Copy link
@parente

parente Feb 7, 2017

Collaborator

Hehe. 👍

@parente parente merged commit 3337c24 into Valassis-Digital-Media:master Feb 7, 2017
@ericdill ericdill deleted the ericdill:handle-fully-qualified-channels branch Feb 7, 2017
@ericdill

This comment has been minimized.

Copy link
Contributor Author

ericdill commented Feb 7, 2017

Thanks @parente

@ericdill ericdill referenced this pull request Feb 7, 2017
@ericdill

This comment has been minimized.

Copy link
Contributor Author

ericdill commented Feb 7, 2017

@dmarkwat 0.6.2 is tagged and on pypi that contains these changes. please give it a spin and let me know if anything is broken! There are some new logging features that allow you to watch for specific lines. "WARNING:" lines show only packages that failed validation. "ERROR:" lines are unexpected stack traces and so should be monitored for. You'll probably want to update your cron scripts to include a "-v" flag to enable WARNING level messages

@dmarkwat

This comment has been minimized.

Copy link

dmarkwat commented Feb 9, 2017

Just ran the mirrors today and everything is looking great. Actually ran it a number of times and it works as expected. Flipped on the debug logging so I know that works, too. Thanks again!

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.