icesat2py / icepyx

Python tools for obtaining and working with ICESat-2 data
https://icepyx.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
203 stars 101 forks source link

Release v0.8.0 #447

Closed rwegener2 closed 10 months ago

rwegener2 commented 10 months ago

A minor release of icepyx! 🎉❄️

review-notebook-app[bot] commented 10 months ago

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

github-actions[bot] commented 10 months ago

Binder :point_left: Launch a binder notebook on this branch for commit eec037e4d1a8ea3dd0df04380fd1d6ce187a5298

I will automatically update this comment whenever this PR is modified

rwegener2 commented 10 months ago

The PR build failed due to login credentials. It seems from the build report that the NSIDC_LOGIN env variable wasn't set? Relevant pieces of the build report: line 4140-4141:

0.00s$ export NSIDC_LOGIN=$NSIDC_LOGIN
The command "export NSIDC_LOGIN=$NSIDC_LOGIN" exited with 0.

and line 4167 and on:

---------------------------- Captured stdout setup -----------------------------
Authentication with Earthdata Login failed with:
{"error":"invalid_credentials","error_description":"Invalid user credentials"}
EARTHDATA_USERNAME and EARTHDATA_PASSWORD are not set in the current environment, try setting them or use a different strategy (netrc, interactive)
You're now authenticated with NASA Earthdata Login
Using token with expiration date: 10/06/2023
Using .netrc file for EDL

Anyone have ideas about this?

JessicaS11 commented 10 months ago

The command "export NSIDC_LOGIN=$NSIDC_LOGIN" exited with 0.

0 indicates it successfully ran, and the authentication does eventually go through, just not on the first try (so earthdata reports a failure then a success). I wonder if we need to be explicit in creating our session to use .netcdf so it logs in the first time and the "bad" output isn't what's saved as out?

JessicaS11 commented 10 months ago

The last successful build of main I can find is 0.6.4, so prior to introduction of earthaccess. I'm remembering now that we were on a time crunch so we merged/released to 0.7.0 despite the failing behind-login tests, since it was clearly an auth issue and everything else worked.

The options as I see them are: 1) merge/release 0.8.0 anyway and add fixing this to the to-do list for 1.0.0; 2) figure out what the auth issue is and delay releasing 0.8.0 until we've fixed that.

@rwegener2 Thoughts?

rwegener2 commented 10 months ago

Ah, that's helpful background @JessicaS11. It is odd to me that this merge to main is failing even though development merges work fine.

Since it's already been done once maybe we just release anyway. The risk, as I see it, is that it doesn't work and we're shipping a broken package, but if it was ok in the last release then odds are good it is fine this time, too (living life on the edge, here). We know of at least one user that is waiting on a release of some of the content, so I suppose my vote is to ship it? This can perhaps serve as another push to invest some time in tests.

And on a technical note:

0 indicates it successfully ran, and the authentication does eventually go through

Good point. I am confused about the "failing then succeeding" cycle, so we should circle back to that.

Update: I started a ticket (#448). Feel free to add/edit if I didn't hit the core of the issue.

JessicaS11 commented 10 months ago

this merge to main is failing even though development merges work fine

This is because the behind_NSIDC_login test suite is only run on the main branch (so in the list of checks, the "branch" test run passes, but when the additional tests are triggered for the "pull request" (what the merged code will look like) it fails).

I am confused about the "failing then succeeding" cycle, so we should circle back to that.

Agreed. No idea if it's relevant for solving the problem, either.