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

Start Harmony-py subset example with PO.DAAC data #187

Merged
merged 11 commits into from
May 23, 2023
Merged

Conversation

jules32
Copy link
Contributor

@jules32 jules32 commented Mar 30, 2023

@asteiker I'm creating a draft PR to surface your work in progress a bit more

@asteiker asteiker requested a review from owenlittlejohns May 11, 2023 20:06
@asteiker
Copy link
Contributor

@jules32 @owenlittlejohns The Harmony team jumped on a quick fix to address the 403 error yesterday. It is now working end to end and I updated subset.qmd accordingly. I do see that we have a separate plotting.qmd so we may be encroaching with the final plotting line in this how-to, but I kept it anyway (perhaps we could leverage this when we get to the plotting how-to and move some of those steps over there). Otherwise I think this can be merged if you think it looks okay.

@asteiker asteiker marked this pull request as ready for review May 11, 2023 20:09
@jules32
Copy link
Contributor Author

jules32 commented May 11, 2023

Great Amy, and I think you've said a good approach for the plotting.

@owenlittlejohns would you like to review for the harmony-py and merge? I can then double-check the quarto build.

@owenlittlejohns
Copy link
Collaborator

The code looks good to me! The request ran, but I'm still getting the 403 when trying to access the data from the 2i2c Hub. Is the Harmony fix a code change that needs to be deployed to production? (For what it's worth the code to get credentials and open in xarray via s3fs also looks solid.)

Also, when merging do you guys have any kind of policy on squashing commits, etc? Or just merging them all as they are in the branch?

@jules32
Copy link
Contributor Author

jules32 commented May 12, 2023

I'll let Amy respond to your code question -

For our reviews/merge, we currently don't have a policy about commits & merging, other than review is required. If you have ideas about squashing we'd love to learn and implement! We'll be formalizing this more this year

@asteiker
Copy link
Contributor

@owenlittlejohns I can reproduce the 403 again as well. It was working yesterday in PROD. I'll work with Harmony to see if something regressed - hopefully we can get it sorted out soon.

@owenlittlejohns
Copy link
Collaborator

Okay, @asteiker - I'll hold of re-testing until I get the word on Harmony being sorted in Prod.

@jules32 - with squashing on merge: that's something I've gotten really into over the last year or so, thanks to Matt Savoie. I think it's potentially a great thing for branches like this were there are a lot of WIP-style commits. GitHub is pretty great in giving you a couple of merge options. If you choose "Squash and Merge" it allows you to set the commit message for the final single commit, which defaults (I think) to the PR title. But you can edit that to something super informative. Plus I love the more atomic commit history you get.

@jules32
Copy link
Contributor Author

jules32 commented May 17, 2023

Thanks @owenlittlejohns , I'd love to set up a time for you to teach your favorite collaborative github/review tips, maybe in the Fall with Mentors onboarding? We can help design with you :)

Copy link
Collaborator

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

I re-attempted this request and the retrieval of the data via s3fs this morning and it all worked! 🎉

@jules32
Copy link
Contributor Author

jules32 commented May 22, 2023

Great! Are you both all set to merge?

@asteiker
Copy link
Contributor

@jules32 Yes feel free to merge! The Harmony team made some updates to ensure the s3 access changes are permanent now (there was a mishap that caused it to regress initially).

@jules32 jules32 merged commit bd6de29 into main May 23, 2023
@jules32 jules32 deleted the harmony-transform branch May 23, 2023 04:23
@jules32
Copy link
Contributor Author

jules32 commented May 23, 2023

Great! It's built with the Cookbook now: 🎉 https://nasa-openscapes.github.io/earthdata-cloud-cookbook/how-tos/subset.html

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.

3 participants