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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

馃挌 Migrate CI from CircleCI to GHA #10

Merged
merged 7 commits into from Dec 7, 2020
Merged

馃挌 Migrate CI from CircleCI to GHA #10

merged 7 commits into from Dec 7, 2020

Conversation

andersy005
Copy link
Member

No description provided.

@martindurant
Copy link
Member

Thank you!
Do we need to remove the circle build, or will merging this be enough?

@andersy005
Copy link
Member Author

Do we need to remove the circle build, or will merging this be enough?

It looks like we don't need it anymore... I just removed it..

@martindurant
Copy link
Member

assert isinstance(entry, intake.catalog.local.LocalCatalogEntry) - this will be a DataSource as written

and it looks like aiohttp need to be added to the installed env.

@andersy005
Copy link
Member Author

Thank you for the hints, @martindurant! Some tests are still failing with this error:

ValueError: open_local can only be used on a filesystem which has attribute local_file=True

I'm looking into it.

martindurant
martindurant reviewed Dec 7, 2020
View reviewed changes
tests/test_cat.py
d = entry.describe()
assert d['name'] == 'err.mnmean.v3.nc'
assert d['container'] == 'xarray'
assert d['plugin'] == ['netcdf']
assert (
d['args']['urlpath']
== 'https://psl.noaa.gov/psd/thredds/dodsC/Datasets/noaa.ersst/err.mnmean.v3.nc'
== 'https://psl.noaa.gov/thredds/dodsC/Datasets/noaa.ersst/err.mnmean.v3.nc'
Copy link
Member

Choose a reason for hiding this comment

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

For the future, we may wish to run a local thredds service instead of a real connection, e.g., https://github.com/Unidata/thredds-docker

@andersy005 andersy005 mentioned this pull request Dec 7, 2020
Open
@martindurant
Copy link
Member

That is the CDF/DAP dichotomy. DAP can open directly from a HTTP URL (but can have a cdf engine), CDF wants to access local files or those that can be made local because of file caching.

@andersy005
Copy link
Member Author

Thank you for the clarification! I hadn't seen that @aaronspring was working on a fix for this issue... Should I leave this as is and let Aaron finish the work he started in #8?

@martindurant
Copy link
Member

Up to you, @aaronspring

@aaronspring
Copy link
Collaborator

I dont mind. there is little overlap with my PRs. you go first. I'll rebase.

@aaronspring
Copy link
Collaborator

I speeded up the testing which took ages.

@martindurant
Copy link
Member

OK, so merging this, and @aaronspring let me know when you are ready, then I can clean up any failure that's left.

@martindurant martindurant merged commit e799112 into master Dec 7, 2020
@andersy005 andersy005 deleted the migrate-to-github-action branch December 7, 2020 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martindurant martindurant Awaiting requested review from martindurant

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

Successfully merging this pull request may close these issues.

None yet
3 participants
@andersy005 @martindurant @aaronspring