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

KMS-516: Implement CRUD for SKOS concepts #10

Merged
merged 22 commits into from
Feb 14, 2025
Merged

KMS-516: Implement CRUD for SKOS concepts #10

merged 22 commits into from
Feb 14, 2025

Conversation

cgokey
Copy link
Contributor

@cgokey cgokey commented Feb 13, 2025

Overview

What is the feature?

This PR implements comprehensive infrastructure changes for deploying the RDF4J database to NGAP, including a switch to EBS for improved performance and migration to a traditional load balancer setup, while also adding CRUD operations for concept management and support for SKOS RDF output. Additionally, it introduces local development improvements and serverless application deployment scripts, enhancing both the production and development environments.

What is the Solution?

Few things in this PR.

  1. Infrastructure code changes for deploying to the RDF4J database to NGAP. These include switching file system to EBS instead of EFS to reduce latency. Also moved from Fargate to a traditional load balancer, ECS service. IAM roles have been updated to reflect this.

  2. Infrastructure code changes to install locally. See README for installing the database and running the application.

  3. Scripts for deploying the serverless application and providing access to the database. Note, the RDFDB outputs cf:rdf4jLoadBalancerStack.RDF4JServiceUrl so it knows where to access the service.

  4. CRUD for handling concept creation, updating, deleting, and fetching.

  5. Adding support for outputing SKOS RDF. This is different than the default output for standard RDF. There was actually a separate ticket for this that I forgot (KMS-527) would be done later, so we should be able to close this ticket after reviewing this PR.

What areas of the application does this impact?

CRUD and Infrastructure

Testing

Definitely test setting up your local environment, make sure I didn't miss any steps.
Once deployed, we should test every endpoint and compare it to KMS. These include:
GET https://cmr.sit.earthdata.nasa.gov/kms/concept/[uuid] (retrieve)
PUT https://cmr.sit.earthdata.nasa.gov/kms/concept/[uuid] (create)
POST https://cmr.sit.earthdata.nasa.gov/kms/concept[uuid] (update)
DELETE https://cmr.sit.earthdata.nasa.gov/kms/concept[uuid] (delete)
There is another endpoint to retrieve the first 2000 concepts. We still need to support paging.
https://cmr.sit.earthdata.nasa.gov/kms/concepts

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests.

Thanks for integrating Codecov - We've got you covered ☂️

@cgokey cgokey requested a review from TylerHeald1 February 13, 2025 19:49
USER tomcat

# Use the new entrypoint script
CMD ["/usr/local/tomcat/setup.sh"]
Copy link

Choose a reason for hiding this comment

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

a .editorconfig file will stop this from happening

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried adding .editorconfig with just Dockerfile and some other ones, but no luck getting vscode to recognize it.

Copy link

Choose a reason for hiding this comment

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

o vscode might need a plugin. Visual studio is the one with built in support, I don't know the difference.

@cgokey cgokey requested a review from jceaser February 14, 2025 14:33

# Install AWS CLI
echo "Installing AWS CLI..."
curl "https://awscli.amazonaws.com/awscli-exe-linux-x86_64.zip" -o "awscliv2.zip"
Copy link

Choose a reason for hiding this comment

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

so wearing my security hat, do we need to be checking a sha hash code or anything? Normally we get some security when we use a package manager, but you're just downloading anything from the net. Anyone spoofing a URL could trick this into downloading something else. Can you grab a specific version and hard code the hash code in the script?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

const result = await createConcepts(event)

expect(result).toEqual({
statusCode: 500,
Copy link

Choose a reason for hiding this comment

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

same as above? 422 or some other 4xx number?

console.error('Error loading RDF XML into RDF4J:', error)

return {
statusCode: 500,
Copy link

Choose a reason for hiding this comment

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

I don't think software should ever have 500.

body: xml,
headers: {
...defaultResponseHeaders,
'Content-Type': 'application/xml; charset=utf-8'
Copy link

Choose a reason for hiding this comment

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

is this line because of my help?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, browser needs application/xml charset=utf-8 to handle special chars and display it as xml

Copy link

@jceaser jceaser left a comment

Choose a reason for hiding this comment

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

I approve, but I think a future release should consider not trusting some random zip file on the web. I also think that 500 errors are not informative for a client and that if there is a 4xx error that is descriptive of the problem, and there is something the client could do like send better data, then one of those codes should be used. A future release should consider that before consumption of these 500s takes place.

@cgokey cgokey merged commit e382742 into main Feb 14, 2025
4 checks passed
@cgokey cgokey deleted the KMS-516 branch February 14, 2025 15:52
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