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 GHA to convert CDL <-> NC #70

Merged
merged 2 commits into from
May 1, 2024
Merged

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Apr 25, 2024

  • Convert .nc files in the datasets folder to and from .cdl if the one or the other is not present.
  • Add dependabot to keep the GitHub Action updated.
  • Add a commit new file option [1].

Below is how the file tree will look like with this option. A Diffable CDL and the binary netCDF4 file. Contributor can commit one or the other and this GitHub Action will prepare the missing part. The decision we need to make on [1] is:

a. We amend the contributor PR with the new file: Elegant, few PRs, but can lead to confusion b/c we will be writing over something that is probably in flux and some contributors are not too familiar with GitHub to work that out.
b. Send a fresh PR from main so the pending file will be added as soon as the PR adding the file was merged.
c. Do not add any file, just fail the PR and ask the contributor to convert and commit it.

I'm inclined to b or c.

.
├── ArchiveADataset.sh
├── convert_nc_cdl.py
├── DasDds.sh
├── datasets
│   ├── edu_calpoly_marine_morro_bay_met.cdl
│   ├── edu_calpoly_marine_morro_bay_met.nc
│   ├── org_cormp_cap2.cdl
│   ├── org_cormp_cap2.nc
│   ├── usf_comps_c10_inwater.cdl
│   └── usf_comps_c10_inwater.nc
├── docker-compose.yml
├── erddap
│   ├── conf
│   │   ├── config.sh
│   │   └── robots.txt
│   └── content
│       ├── datasets.xml
│       ├── images
│       │   └── erddap2.css
│       └── setup.xml
├── erddap_utils.sh
├── GenerateDatasetsXml.sh
└── README.md

@ocefpaf
Copy link
Member Author

ocefpaf commented Apr 25, 2024

I'd like to wait for @srstsavage comments on #65 (comment) before we commit to this path.

@mwengren
Copy link
Member

@ocefpaf I like the b option as well.

Also a, but I agree that adding commits to someone's PR branch, assuming that's what would happen, might lead to confusion if someone who isn't git-savvy tries to push more commits from their local clone/PR branch.

@srstsavage
Copy link
Contributor

This approach using CDL for text representation looks great! I think I favor b (additional PR adding missing data file) as well as long as it works as described, or c (fail if missing) with the needed command to generate the missing file in the error message if b is problematic.

@MathewBiddle
Copy link
Contributor

I'm in favor of b as well and agree with the caveats above.

@ocefpaf ocefpaf marked this pull request as ready for review May 1, 2024 18:02
@ocefpaf
Copy link
Member Author

ocefpaf commented May 1, 2024

I'm in favor of b as well and agree with the caveats above.

Option b should work. If not we revert to option c. We need to merge this and add a file to test it 😬

PS: @MathewBiddle it is similar to what we did in the metrics repository in ioos/ioos_metrics#65

@mwengren
Copy link
Member

mwengren commented May 1, 2024

@ocefpaf I can't say I understand the GHA code at quick glance, but if you say this is good as is with the latest commit for option b, I'm going to merge it.

We could test this by moving existing datasets under a hierarchy like I started to do by adding the /datasets/gliders/ directory in #66. I'll see about submitting another PR to do that one of these days.

@mwengren mwengren merged commit 1d435af into ioos:main May 1, 2024
1 check passed
@mwengren
Copy link
Member

mwengren commented May 1, 2024

That's awesome, it ran already by picking up the existing .nc files without equivalent .cdl: #73! Looks to be working well!

@ocefpaf ocefpaf deleted the GHA_nc_cdl_conversion branch May 1, 2024 19:28
@ocefpaf
Copy link
Member Author

ocefpaf commented May 1, 2024

We could test this by moving existing datasets under a hierarchy like I started to do by adding the /datasets/gliders/ directory in #66. I'll see about submitting another PR to do that one of these days.

The current script I'm using to convert will fail in that scenario but it should be an easy fix. We don't look at any subdirectories at the moment.

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.

4 participants