-
Notifications
You must be signed in to change notification settings - Fork 2
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
added CF override for the metadata bug in ATL19 in crs_txt #7
Conversation
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.
Nice work! I only had some minor comments about the release notes. Nothing is a show-stopper.
Because there are a couple of commits in this PR, it'd be good if you could do a couple of things when you merge:
- Use the "Squash and Merge" option you can get from the dropdown menu of the Merge button.
- Reformat the commit message at that point so it adheres to our contribution guidelines. It should end up looking something like: "DAS-2106 - Add CF Override for ATL19 crs_txt." (The important bit is prepending the commit message with the ticket number)
CHANGELOG.md
Outdated
@@ -1,3 +1,11 @@ | |||
|
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.
A couple of nits with this description:
- The issue is sort of a leading backslash, but really it's a leading speech mark that is escaped by a backslash.
- There's a stray blank line at the top of the file (that shouldn't affect the CI/CD trying to create the release notes, but I'd get rid of it just to be safe).
This causes the errors
, maybe instead:This causes errors
.- There's a missing full stop at the end of the paragraph.
I don't think any of these are showstoppers. Just me being a bit pedantic. (I tested the bin/extract-release-notes.sh
script with a blank line at the top of CHANGELOG.md
and it worked, but had the blank line that might look a bit odd)
{ | ||
"Name": "crs_wkt", | ||
"Value": "PROJCS[\"NSIDC Sea Ice Polar Stereographic North\",GEOGCS[\"Unspecified datum based upon the Hughes 1980 ellipsoid\",DATUM[\"Not_specified_based_on_Hughes_1980_ellipsoid\",SPHEROID[\"Hughes 1980\",6378273,298.279411123061,AUTHORITY[\"EPSG\",\"7058\"]],AUTHORITY[\"EPSG\",\"6054\"]],PRIMEM[\"Greenwich\",0,AUTHORITY[\"EPSG\",\"8901\"]],UNIT[\"degree\",0.0174532925199433,AUTHORITY[\"EPSG\",\"9122\"]],AUTHORITY[\"EPSG\",\"4054\"]],PROJECTION[\"Polar_Stereographic\"],PARAMETER[\"latitude_of_origin\",70],PARAMETER[\"central_meridian\",-45],PARAMETER[\"scale_factor\",1],PARAMETER[\"false_easting\",0],PARAMETER[\"false_northing\",0],UNIT[\"metre\",1,AUTHORITY[\"EPSG\",\"9001\"]],AXIS[\"X\",EAST],AXIS[\"Y\",NORTH],AUTHORITY[\"EPSG\",\"3411\"]]" | ||
} |
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.
Nice. This looks a bit clunky, because the value of the metadata attribute it looooong, but I think this is a good fix. When the collection is fixed up, we'll be able to remove this anyway and clean up the file.
Description
A short description of the changes in this PR.
Jira Issue ID
DAS-2106
Local Test Steps
http://localhost:3000/C1260128940-EEDTEST/ogc-api-coverages/1.0.0/collections/north_polar%2Fice_conc_albm,mid_latitude%2Fice_conc_albm,south_polar%2Fice_conc_albm/coverage/rangeset?forceAsync=true&subset=lat(-78.2%3A82.7)&subset=lon(-101.4%3A72.9)&granuleId=G1260128994-EEDTEST&maxResults=1
PR Acceptance Checklist
CHANGELOG.md
updated to include high level summary of PR changes.VERSION
updated if publishing a release.