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.

Sign up for GitHub

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

Jump to bottom

Bug Fix: Populate Dataset access_urls #736

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft

Bug Fix: Populate Dataset access_urls #736

wants to merge 5 commits into from

Conversation

jamespolly
Copy link

@jamespolly jamespolly commented Sep 22, 2023 •
edited

Description Of Changes

This is an incremental PR that will need some attention to testing and additional input. Intended to get the ball rolling.

Minor change to catalog.py which permits populating Dataset.access_urls in situations where service_name is not in all_service_dict.

See #734 and #715.

38 tests were failing prior to my change. 41 tests failed afterward. Common themes across the three new failures:

> FAILED tests/test_catalog.py::test_simple_service_within_compound - AssertionError: assert {'OPENDAP': '...20170824.txt'} == {'HTTPServer'...20170824.txt'}
22a24,25
> FAILED tests/test_catalog_access.py::test_case_insensitive_access - AssertionError: assert 'OPENDAP' == 'HTTPSERVER'
> FAILED tests/test_catalog_access.py::test_manage_access_types_case_insensitive - AssertionError: assert {'OPENDAP': '...20170824.txt'} == {'HTTPSERVER'...20170824.txt'}

OPeNDAP is chosen as a first (and sometimes only) access_url rather than HTTPServer. Oddly I cannot replicate these assertion errors in ipython.

Checklist

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@jamespolly jamespolly marked this pull request as draft September 22, 2023 14:23
@jamespolly
Copy link
Author

Taking another stab to get the tests passing in the CI. My opening comment in this PR mentioned 38 test failing on my machine when following the test instructions. Only three tests failed in the CI so there's clearly a difference in these testing implementations.

In any event trying to get those failed tests passing so this can be reviewed.

@jamespolly
Copy link
Author

@dopplershift Can I rerun the CI here or is that beyond my permissions?

@dopplershift
Copy link
Member

Just kicked them off, thanks for the poke.

@jamespolly
Copy link
Author

@dopplershift Well this was embarrassing. Once more time would be appreciated.

@jamespolly
Copy link
Author

Thank you @dopplershift. Looks like flake8 and tests are passing. There are some other checks not passing (Docs Build) but I don't think I changed anything here.

Please let me know if that is something I should fix!

@jamespolly
Copy link
Author

@dopplershift Looks like there are some lower-level build failures (numpy and cartopy) that are happening in the CI. Have we seen these before? Seems unrelated to the changes made in this PR. Re-poking just incase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dopplershift dopplershift Awaiting requested review from dopplershift dopplershift is a code owner

@lesserwhirls lesserwhirls Awaiting requested review from lesserwhirls lesserwhirls is a code owner

@dcamron dcamron Awaiting requested review from dcamron dcamron is a code owner

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No access urls created for Dataset
4 participants
@jamespolly @CLAassistant @dopplershift @jpollygda