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

Artifacts fix #101

Merged
merged 12 commits into from
Apr 24, 2017
Merged

Artifacts fix #101

merged 12 commits into from
Apr 24, 2017

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Feb 14, 2017

See ome/omero-marshal#36 (comment)

As part of the intermediate release of OMERO 5.3.0-m8, the Ice 3.5 artifacts were removed from the landing page. This caused unwanted effects on consumers of omego like the omero-marshal Travis build.

The root of the issue is that for release artifacts omego download is using the content of the index page for listing the artifacts. The logic is very fragile and easily affected by changes to the page content.

This PR proposes to use the canonical <omero_downloads_url>/artifacts URL instead as our OMERO downloads pages have been following this convention for many years now. This should provide a listing of all the artifacts and allow regexp to filter as desired.

To test this PR functionally, check that

omego download --ice 3.5 --release 5.3 py [-n]

is currently broken while with this PR

python omego/main.py download --ice 3.5 --release 5.3 py [-n]

works as expected.

Depending on the needs, we might want an emergency release of omego with this patch although i'd like to look into ways to unit testing this detection logic. Also this has been largely tested in the context of OMERO 5.3 and the download command but it might be good to review the impact on other usages of omego /cc @manics @jburel @joshmoore @aleksandra-tarkowska

@manics
Copy link
Member

manics commented Feb 14, 2017

This will break the IDR which uses a custom downloads page: https://github.com/IDR/deployment/blob/master/ansible/group_vars/omero-hosts.yml#L12

Easy to fix (we can just add an extra artifacts directory to the IDR downloads site), but as you've realised we don't officially define the URL and HTML structure of the downloads site anywhere. http://downloads.openmicroscopy.org/omero/5.2.7/artifacts/ still requires parsing the HTML.

@atarkowska
Copy link
Member

works as expected

(test) ls31620:TEST ola$ pip install -U git+git://github.com/sbesson/omego.git@artifacts_fix#egg=omego
Collecting omego from git+git://github.com/sbesson/omego.git@artifacts_fix#egg=omego
  Cloning git://github.com/sbesson/omego.git (to artifacts_fix) to /private/var/folders/xl/jj4w78c127n_2z4ww0zyrvf80000gn/T/pip-build-sOKHMb/omego
Collecting yaclifw>=0.1.1 (from omego)
Collecting argparse (from yaclifw>=0.1.1->omego)
  Using cached argparse-1.4.0-py2.py3-none-any.whl
Installing collected packages: argparse, yaclifw, omego
  Running setup.py install for omego ... done
Successfully installed argparse-1.4.0 omego-0.3.0-56-g2271 yaclifw-0.1.2
(test) ls31620:TEST ola$ omego download --ice 3.5 --release 5.3 py 
2017-02-14 18:54:54,561 [omego.artifa] INFO  Checking http://downloads.openmicroscopy.org/omero/5.3.0-m8/artifacts/OMERO.py-5.3.0-m8-ice35-b44.zip
2017-02-14 18:54:54,562 [omego.fileut] INFO  Downloading http://downloads.openmicroscopy.org/omero/5.3.0-m8/artifacts/OMERO.py-5.3.0-m8-ice35-b44.zip
2017-02-14 18:54:56,185 [omego.artifa] INFO  Unzipping OMERO.py-5.3.0-m8-ice35-b44.zip
(test) ls31620:TEST ola$ OMEGO_SSL_NO_VERIFY=1 omego download py --httpuser *** --httppassword *** --branch OMERO-build --ci https://10.0.51.135:8443
2017-02-14 18:55:15,153 [omego.artifa] INFO  Checking https://10.0.51.135:8443/job/OMERO-build/lastSuccessfulBuild/artifact/src/target/OMERO.py-5.2.3-436-3a6e84a-ice36-b60.zip
2017-02-14 18:55:15,153 [omego.fileut] INFO  Downloading https://10.0.51.135:8443/job/OMERO-build/lastSuccessfulBuild/artifact/src/target/OMERO.py-5.2.3-436-3a6e84a-ice36-b60.zip
2017-02-14 18:55:18,229 [omego.artifa] INFO  Unzipping OMERO.py-5.2.3-436-3a6e84a-ice36-b60.zip 

@atarkowska
Copy link
Member

atarkowska commented Feb 14, 2017

how about if --downloadurl it will skip that fix?

@sbesson
Copy link
Member Author

sbesson commented Feb 14, 2017

Thanks both. As pointed out by Travis, unit tests would need to be updated anyways to reflect the new changes. But it's good to hear about other use cases. Agreed with @manics that the library is currently working around the fact our downloads pages lack an official layout. Considering turning this into an issue first to discuss the various options.

@sbesson
Copy link
Member Author

sbesson commented Apr 21, 2017

@manics @joshmoore @jburel: the last commits should fix all failing unit and integration tests, add the missing changelog for 0.5.0 and add documentation for this breaking bug fix both in the changelog and the help displayed by omego download -h.

As mentioned elsewhere, any consumer of the omego download API using a custom downloads server like IDR might have to adjust the structure of their downloads server to allow artifacts to be available under the canonical DOWNLOADS_URL/omero/VERSION/artifacts. Alternatively, we could consider adding a --legacy_downloads flag allowing to restore the pre-0.6.0 behavior but this would require downstream modifications anyways to pass the appropriate flag.

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

A couple of minor points.

I don't think the additional complexity of --legacy_downloads is worthwhile.

@@ -431,7 +431,8 @@ def read_downloads(dlurl):

class DownloadCommand(Command):
"""
Download an OMERO artifact from a CI server.
Download an OMERO artifact from either a donwloads or a Continous
Copy link
Member

Choose a reason for hiding this comment

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

typo: donwloads and Continous

omego/env.py Outdated
Add(group, "labels", "",
help="Comma separated list of labels for matrix builds")
help="Comma separated list of labels for matrix builds (CI only)")

Add(group, "downloadurl",
"http://downloads.openmicroscopy.org",
Copy link
Member

Choose a reason for hiding this comment

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

This seems like a good time to change to https

@joshmoore
Copy link
Member

@manics: worth getting this in and seeing if gh-103 goes green?

@sbesson sbesson modified the milestone: 0.6.0 Apr 24, 2017
@joshmoore
Copy link
Member

Merging and re-opening gh-103 as candidates for 0.6.0

@joshmoore joshmoore merged commit d6c6bb1 into ome:master Apr 24, 2017
@sbesson sbesson deleted the artifacts_fix branch April 24, 2017 08:47
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.

4 participants