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

Add S3 support #180

Merged
merged 4 commits into from
Jul 17, 2023
Merged

Add S3 support #180

merged 4 commits into from
Jul 17, 2023

Conversation

zklaus
Copy link

@zklaus zklaus commented Jul 6, 2023

Checklist

  • Used a personal fork of the feedstock to propose changes
  • Bumped the build number (if the version is unchanged)
  • Reset the build number to 0 (if the version changed)
  • Re-rendered with the latest conda-smithy (Use the phrase @conda-forge-admin, please rerender in a comment in this PR for automated rerendering)
  • Ensured the license file is being packaged.

This is a straight-up port of @djhoese's PR to add S3 support, rebased onto the latest main.

@conda-forge-webservices
Copy link

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe) and found it was in an excellent condition.

@zklaus
Copy link
Author

zklaus commented Jul 6, 2023

@conda-forge-admin, please rerender

@djhoese
Copy link
Contributor

djhoese commented Jul 6, 2023

I'll close #160 in favor of this. Note that this was based on work by others including @ocefpaf in #125 so if something is wrong with these flags I'm not sure I'm up to date on how it should work/change.

@djhoese djhoese mentioned this pull request Jul 6, 2023
5 tasks
@zklaus
Copy link
Author

zklaus commented Jul 7, 2023

I had to deactivate the tests for Zarr to make this work, but note that this seems to be mostly due to permissions. I will try to give an overview here of the failing tests and the reasons for the failures, taken from the last attempted run before switching off the tests.

The pattern is always the same: The failing tests test a bunch of stuff with Zarr files and ultimately try to write a temporary file to an AWS bucket to which we don't have write access. Here I list all the failing tests with links to the line that names the object key to which the access is denied.

@zklaus zklaus changed the title Feature s3 Add S3 support Jul 7, 2023
@zklaus
Copy link
Author

zklaus commented Jul 7, 2023

I think this is ready as is; we might also have a discussion about how to make the tests work.

@dopplershift
Copy link
Member

@DennisHeimbigner @WardF Can you all shed any light on those tests? Is there a correct incantation that non-Unidata folk can use to only run zarr tests for public data?

@DennisHeimbigner
Copy link

First, what actual netcdf-c version are you using? 4.9.2, I assume.

@dopplershift
Copy link
Member

Yes, this PR is adding S3 support to the conda-forge builds for netcdf-c 4.9.2.

@DennisHeimbigner
Copy link

4.9.2 does not support S3 testing yet unless you have access to the Unidata S3 private bucket.

@zklaus
Copy link
Author

zklaus commented Jul 10, 2023

@DennisHeimbigner, thanks, that's right. Do you object to the proposed course of action of merging the support here without re-activating the tests?

@DennisHeimbigner
Copy link

No objection on my part -- it your code. I will note that there are significant
changes in the handling of S3 testing in the next netcdf-c release, so you
may have to do this all over again.

@zklaus
Copy link
Author

zklaus commented Jul 11, 2023

Thanks, @DennisHeimbigner, sounds good. @dopplershift, is it ok for you to move forward here?

@valeriupredoi
Copy link

hi @dopplershift just wanted to drop by thank @zklaus for this and also say we're eagerly awaiting this to be plopped in too 😃 🦅

@dopplershift dopplershift merged commit 71e9afd into conda-forge:main Jul 17, 2023
17 checks passed
@zklaus zklaus deleted the feature-s3 branch July 18, 2023 08:39
@zklaus
Copy link
Author

zklaus commented Jul 18, 2023

Cheers and thanks everyone! 🎉

@valeriupredoi
Copy link

awesome, very many thanks @zklaus and @dopplershift 🍺 x2

@zklaus zklaus mentioned this pull request Jul 18, 2023
5 tasks
@akrherz akrherz mentioned this pull request Aug 29, 2023
4 tasks
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.

6 participants