ioos / erddap-gold-standard

Contains the 'gold standard' ERDDAP configuration, with datasets compliant with IOOS Metadata Profile 1.2
https://standards.sensors.ioos.us/erddap/index.html
8 stars 16 forks source link

add GHA to convert CDL <-> NC #70

Closed ocefpaf closed 2 months ago

ocefpaf commented 2 months ago

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 commented 2 months ago

I'd like to wait for @srstsavage comments on https://github.com/ioos/erddap-gold-standard/pull/65#issuecomment-1856392957 before we commit to this path.

mwengren commented 2 months ago

@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 commented 2 months ago

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 commented 2 months ago

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

ocefpaf commented 2 months ago

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 https://github.com/ioos/ioos_metrics/pull/65

mwengren commented 2 months ago

@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 https://github.com/ioos/erddap-gold-standard/pull/66. I'll see about submitting another PR to do that one of these days.

mwengren commented 2 months ago

That's awesome, it ran already by picking up the existing .nc files without equivalent .cdl: https://github.com/ioos/erddap-gold-standard/pull/73! Looks to be working well!

ocefpaf commented 2 months ago

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.