-
Notifications
You must be signed in to change notification settings - Fork 79
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
Use temp .netrc file for integration tests and support NETRC environment variable #808
base: main
Are you sure you want to change the base?
Conversation
2f1bd8a
to
98ba9bb
Compare
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #808 +/- ##
=======================================
Coverage ? 69.88%
=======================================
Files ? 31
Lines ? 2115
Branches ? 0
=======================================
Hits ? 1478
Misses ? 637
Partials ? 0 ☔ View full report in Codecov by Sentry. |
98ba9bb
to
9fea543
Compare
# | ||
# ORNLDAAC no longer has any on-prem collections. This returns 0 collections: | ||
# https://cmr.earthdata.nasa.gov/search/collections?data_center=ORNL_DAAC&cloud_hosted=false | ||
# The following is commented out because the test in this file will now always fail | ||
# because there are no longer any on-prem collections. | ||
# | ||
# { | ||
# "short_name": "ORNLDAAC", | ||
# "collections_count": 100, | ||
# "collections_sample_size": 3, | ||
# "granules_count": 100, | ||
# "granules_sample_size": 2, | ||
# "granules_max_size_mb": 50, | ||
# }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to remove this completely?
# | ||
# ORNLDAAC no longer has any on-prem collections. This returns 0 collections: | ||
# https://cmr.earthdata.nasa.gov/search/collections?data_center=ORNL_DAAC&cloud_hosted=false | ||
# The following is commented out because the test in this file will now always fail | ||
# because there are no longer any on-prem collections. | ||
# | ||
# { | ||
# "short_name": "ORNLDAAC", | ||
# "collections_count": 100, | ||
# "collections_sample_size": 2, | ||
# "granules_count": 100, | ||
# "granules_sample_size": 2, | ||
# "granules_max_size_mb": 50, | ||
# }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to remove this completely?
Apologies for the relatively large PR. I could perhaps split out the changes for #743, if anybody feels this is too large. I would at least like to keep the changes addressing both #480 and #806 together since the changes for #806 effectively necessitate the changes for #480. This also now fixes #815. It made sense for me to add the changes for that issue so that I could actually run integration tests locally since this issue is precisely for handling integration tests. Regardless, integration tests now run successfully on PRs from forks. For PRs from forks owned by those of use who are maintainers of this repo, the integration tests run successfully (barring actual failing tests, of course) without intervention, as we have write permission. For PRs from forks owned by anybody without write access to this repo, the integration tests job will now fail due to a permission check, not due to failure to read the necessary secrets, but now (once this PR is merged) it will be possible for a maintainer to check the PR, and if all looks safe, re-run the failed job. When this happens, the job re-runs with the maintainer's permissions, and thus the integration tests will run (and ideally succeed as well). |
This primary intent for the PR was to address #806. Because the changes required for #806 also produced at least partial completion of #743 and #480, it also includes the full set of changes for them as well. All 3 issues are very closely related, so it didn't make much sense to attempt to separate them out.
As a result,
~/.netrc
file, if they have one.netrc
file location, it made sense to do so by setting aNETRC
environment variable for this purpose, so users may now also set this environment variable, if they need to specify a different location for their.netrc
fileFixes #806
Fixes #743
Fixes #480
Pull Request (PR) draft checklist - click to expand
contributing documentation
before getting started.
title such as "Add testing details to the contributor section of the README".
example
closes #1
. SeeGitHub docs - Linking a pull request to an issue.
CHANGELOG.md
with details about your change in a section titled## Unreleased
. If such a section does not exist, please create one. FollowCommon Changelog for your additions.
README.md
with details of changes to theearthaccess interface, if any. Consider new environment variables, function names,
decorators, etc.
Click the "Ready for review" button at the bottom of the "Conversation" tab in GitHub
once these requirements are fulfilled. Don't worry if you see any test failures in
GitHub at this point!
Pull Request (PR) merge checklist - click to expand
Please do your best to complete these requirements! If you need help with any of these
requirements, you can ping the
@nsidc/earthaccess-support
team in a comment and wewill help you out!
Request containing "pre-commit.ci autofix" to automate this.
📚 Documentation preview 📚: https://earthaccess--808.org.readthedocs.build/en/808/