Support generic HTTP auth with opendap

intake / intake-xarray

Intake plugin for xarray

https://intake-xarray.readthedocs.io/

BSD 2-Clause "Simplified" License

76 stars 36 forks source link

Support generic HTTP auth with opendap #72

Closed leifdenby closed 4 years ago

leifdenby commented 4 years ago

intake-xarray currently can only authenticate against the OPeNDAP servers provided in https://github.com/pydap/pydap/tree/master/src/pydap/cas. This commit adds an auth option called generic_http for the OPeNDAP driver, following the directions at http://xarray.pydata.org/en/stable/io.html#opendap

martindurant commented 4 years ago

Looks OK to me, although I am no DAP expert - would love a comment from someone who is. Given that this is HTTP only, is it plausible to test directly?

martindurant commented 4 years ago

Also, help appreciated with this likely unrelated error

  File "/home/travis/build/intake/intake-xarray/intake_xarray/netcdf.py", line 57, in __init__
    super(NetCDFSource, self).__init__(metadata=metadata, **kwargs)
TypeError: __init__() got an unexpected keyword argument 'engine'

d70-t commented 4 years ago

@martindurant / unrelated engine error: git bisect says this one broke it.

d70-t commented 4 years ago

... and the engine refers to this location, however I did not validate if other tests fail if that line is commented out.

martindurant commented 4 years ago

Yeah, it looks like it should be

      xarray_kwargs:
        engine: pynio

but why did this work before?

martindurant commented 4 years ago

(perhaps all that happened, is that this entry was never tested, because the backend wasn't installed, and the change in Entry to instantiate the source object revealed the previous bug in the catalog)

d70-t commented 4 years ago

I am pretty new to intake, so I can't really judge this. But I could not find a way how engine would have made it to the xarray open_dataset call before. By applying your change I'd understand how the parameter would make its way through.

martindurant commented 4 years ago

@leifdenby , do you have time to implement the fix in this PR?

d70-t commented 4 years ago

I'd propose to handle it in a separate PR #75 as this seems to be a separate issue.

martindurant commented 4 years ago

OK, so this PR needs to merge from master

weiji14 commented 4 years ago

Haven't tried this branch (authentication is hard to test!) , but the new 'generic_http' auth method should be added to the docstring here:

https://github.com/intake/intake-xarray/blob/97c869ec99fd50b1f06a5c2e5daaf10f07c3afae/intake_xarray/opendap.py#L29-L36

leifdenby commented 4 years ago

@martindurant and @d70-t I can wait until a fix has been merged for the CI tests. Maybe that is best?

thanks @weiji14 I've updated the docstring

d70-t commented 4 years ago

@leifdenby I think that @martindurant suggested that you would merge the current master (which already has the CI fix) into you branch. But maybe github was also clever enough to run the latest PR checks on what would be the result of merging this PR into master, which would essentially result in the same state of the working directory. Anyways, I think this PR should be fine for now.