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

Fix for accessing THREDDS catalogs hosted at a Context Path other tha… #108

Merged
merged 4 commits into from Sep 9, 2016

Conversation

joelrahman
Copy link
Contributor

…n /thredds

I'm using siphon to access several THREDDS instances hosted at nonstandard context paths - i.e. without /thredds/ anywhere in the URL.

The /thredds/ context is hard coded in Siphon and this causes failures in connecting to these catalogs. Basically, siphon takes the entire catalog URL and appends the service URL to the end. (I'm taking the siphon URLs and passing them to pydap)

This is a simple fix that works for every non-standard server I've encountered thus far and passes your tests when run on Python 3.4.

I've worked on the assumption that the service descriptions in the catalog should always include an absolute path, which includes the context path, making it safe to take everything from the URL except the actual path (i.e. take scheme, host, username/password, port). Let me know if you see other cases and I can take a look.

@dopplershift
Copy link
Member

Thanks for this. Are you aware that github is attaching these commits to a different flowmatters account rather than joelrahman?

Could you add a test validating that this works for one of the non-standard context servers? Otherwise, we risk breaking this in the future.

@lesserwhirls Any opinions on this?

@dopplershift
Copy link
Member

I can't find any other mentions of /thredds/ in the codebase like that; and regardless of "correctness", I will say using urlparse seems much better than a simple string split anyway.

@joelrahman
Copy link
Contributor Author

Happy to add a test. I note that all the current catalog tests hit unidata servers. I've got no control over the THREDDS servers I'm using - they could disappear, temporarily or permanently, leaving you with intermittent tests.

Do you want the test to hit a live server, noting those risks, or should I just add a test for _find_base_tds_url?

(Won't get back to this for 24 hours)

Re the accounts - both me sorry... Seems I had my git config messed up when I cloned the fork...

@dopplershift
Copy link
Member

Re: accounts--it's up to you. The association is based on the email address, so you could change your email in your git settings and rewrite the commits (see here). It makes no difference to me unless you want the credit or whatnot on your other account.

For tests, our tests use a library called vcrpy to record and playback http requests. If you add a new test function with a dectorator like:

@recorder.use_cassette('accessing-my-thredds-server')
def test_nonstandard_server():
'Testing server on different context'
    pass

It will record you accessing the server the first time you run the test, and subsequent runs of the test will use the recorded data, called a casette. Just make sure you commit the cassette file, which will be called accessing-my-thredds-server in this case.

@dopplershift
Copy link
Member

That looks great. Thanks for the contribution and for adding the test.

@dopplershift dopplershift merged commit 211e554 into Unidata:master Sep 9, 2016
@dopplershift dopplershift modified the milestone: 0.4.1 Mar 31, 2017
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

2 participants