Skip to content

CUMULUS-5734: Updated getGranuleConceptMetadata to throw on non-404 errors - #4628

Open
jennyhliu wants to merge 8 commits into
masterfrom
jl/CUMULUS-5734
Open

jennyhliu wants to merge 8 commits into
masterfrom
jl/CUMULUS-5734

Conversation

@jennyhliu

@jennyhliu jennyhliu commented Oct 22, 2026 •

Copy link
Copy Markdown
Contributor

Summary: Summary of changes

Addresses CUMULUS-5734: Cumulus Allows Granule Deletion After Failed CMR Removal

Changes

  • Updated @cumulus/cmr-client getGranuleConceptMetadata to throw on non-404 errors.

Unit test for unpublishGranule error case is already covered here

PR Checklist

  • Update CHANGELOG
  • Unit tests
  • Ad-hoc testing - Deploy changes and test manually
  • Integration tests

📝 Note:
For most pull requests, please Squash and merge to maintain a clean and readable commit history.

@jennyhliu jennyhliu added needs review Very Small Look at this one because it's very easy. labels Oct 22, 2026

@npauzenga npauzenga left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks great @jennyhliu! Good find! Just had a couple very minor questions.

Comment thread CHANGELOG.md Outdated
- **CUMULUS-5271**
- Update zod and adding typescript Node dependencies due to Snyk findings/sercurity vulnerabilities
- **CUMULUS-5734**
- Updated `@cumulus/cmr-client` `getGranuleConceptMetadata` to throw on non-404 errors.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should also include the follow-redirects version bump?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we include a note here (or elsewhere) to users about the behavior change? It seems like a possibility that they'll see more errors because of this change and that could be alarming. But it's correct. Those errors should have been surfaced previously.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's ci audit issue Micah is fixing, and he will include in his PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

updated changelog about behavior change.

"@cumulus/logger": "22.4.2",
"ajv": "^6.12.3",
"follow-redirects": "^1.2.4",
"follow-redirects": "^1.16.1",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Was this a vulnerability?

"public-ip": "^3.0.0",
"xml2js": "0.5.0",
"zod": "^3.20.2"
"zod": "^4.6.6"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

match zod version in @cumulus/api/package.json

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

in review Very Small Look at this one because it's very easy.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants