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

DAS-1900: As a developer, I would like to be able to make an HTTPS request that is routed to my variable generation service, so that I can request UMM-Var records are generated in the cloud #63

Merged
merged 32 commits into from
Oct 17, 2023

Conversation

william-valencia
Copy link
Contributor

@william-valencia william-valencia commented Sep 1, 2023

Overview

What is the feature?

DAS-1900: As a developer, I would like to be able to make an HTTPS request that is routed to my variable generation service, so that I can request UMM-Var records are generated in the cloud

We decided that variable generation(earthdata-varinfo) will be routed through graphQL

What is the Solution?

I created an earthdata-varinfo lambda. Also modified the cmr-graphQL to accept a generateVariableDrafts query under the existing collection query, which will return variable drafts generated by the earthdata-varinfo lambda.

What areas of the application does this impact?

Collection query will have a new field(generateVariableDrafts) that can be requested.
Also added a new field for variableDrafts called metadataSpecification.

Testing

Reproduction steps

  • Environment for testing:
  • cmr-graphql
  • Collection to test with:
  • C1598621093-GES_DISC or similar
  1. Navigate to Apollo Studio for the environment you will be working in
  2. Request the following collection query:
query Collection($params: CollectionInput) {
  collection(params: $params) {
    conceptId
    generateVariableDrafts {
      count
      items {
        dataType
        definition
        dimensions
        longName
        name
        standardName
        units
        metadataSpecification
      }
    }
  }
}
  1. Specify the variable input params conceptId ie:
{
  "params": {
    "conceptId": "C1598621093-GES_DISC"
  }
}
  1. Put in your authorization token in the Headers area.
  2. Submit the query and you should receive something like the following back:
{
  "data": {
    "collection": {
      "conceptId": "C1598621093-GES_DISC",
      "generateVariableDrafts": {
        "count": 16,
        "items": [
          {
            "dataType": "int32",
            "definition": "Grid/time",
            "dimensions": [
              {
                "Name": "Grid/time",
                "Size": 1,
                "Type": "TIME_DIMENSION"
              }
            ],
            "longName": "Grid/time",
            "name": "Grid/time",
            "standardName": "time",
            "units": "seconds since 1970-01-01 00:00:00 UTC",
            "metadataSpecification": {
              "URL": "https://cdn.earthdata.nasa.gov/umm/variable/v1.8.2",
              "Name": "UMM-Var",
              "Version": "1.8.2"
            }
          },...
}

Attachments

Screenshot 2023-08-31 at 11 54 15 PM

Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.

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
Copy link

codecov bot commented Sep 1, 2023

Codecov Report

Merging #63 (d956e67) into main (9de0272) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main       #63   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           63        65    +2     
  Lines         1304      1336   +32     
  Branches       176       180    +4     
=========================================
+ Hits          1304      1336   +32     
Files Coverage Δ
src/constants/index.js 100.00% <ø> (ø)
src/datasources/collectionVariableDrafts.js 100.00% <100.00%> (ø)
src/datasources/orderOption.js 100.00% <ø> (ø)
src/resolvers/collection.js 100.00% <100.00%> (ø)
src/resolvers/variableDraft.js 100.00% <100.00%> (ø)
src/utils/aws/getLambdaConfig.js 100.00% <100.00%> (ø)
src/utils/parseRequestedFields.js 100.00% <100.00%> (ø)

src/datasources/GenerateCollectionVariablesAPI.js Outdated Show resolved Hide resolved
src/datasources/GenerateCollectionVariablesAPI.js Outdated Show resolved Hide resolved
src/datasources/GenerateCollectionVariablesAPI.js Outdated Show resolved Hide resolved
src/earthdataVarinfo/handler.py Outdated Show resolved Hide resolved
src/earthdataVarinfo/handler.py Outdated Show resolved Hide resolved
src/earthdataVarinfo/handler.py Outdated Show resolved Hide resolved
src/earthdataVarinfo/handler.py Outdated Show resolved Hide resolved
src/earthdataVarinfo/handler.py Outdated Show resolved Hide resolved
src/graphql/handler.js Outdated Show resolved Hide resolved
src/resolvers/collection.js Outdated Show resolved Hide resolved
@macrouch
Copy link
Contributor

macrouch commented Sep 1, 2023

Is there anything special you need to do to get the python lambda working? Does serverless-python-requirements do everything? Is there a specific python install required on your dev system? I think the readme needs updating with information about the new capability and any specific python information

@william-valencia
Copy link
Contributor Author

Is there anything special you need to do to get the python lambda working? Does serverless-python-requirements do everything? Is there a specific python install required on your dev system? I think the readme needs updating with information about the new capability and any specific python information

Yes I did run the commands below to get things working. Will update the README
python3 -m venv .venv
source .venv/bin/activate
pip install -r requirements.txt

Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

Nice work. Most of my comments are small, including:

  • Trying out a Python 3.11 runtime.
  • Cleaning up the requirements.txt to only include 3rd party packages explicitly imported in handler.py.
  • The context manager thing for the temporary directory clean-up.
  • Running a Python linter/style checker over handler.py to catch some minor things.

Potentially a more important thing is that there is no unit testing of handler.py. I think that's because this is the fist instance of Python in this repository. Not sure what @abbottry thinks about that (given the high test coverage he was just touting).

The only other thing is a PSA about the token handling. I'll try and get that consistent in earthdata-varinfo (for search/download vs the publication) for the next version, which will mean you have a simpler life, albeit with a couple of code changes in the next PR for this repo.

serverless.yml Show resolved Hide resolved
requirements.txt Show resolved Hide resolved
src/datasources/collectionVariableDrafts.js Show resolved Hide resolved
src/earthdataVarinfo/handler.py Show resolved Hide resolved
src/earthdataVarinfo/handler.py Outdated Show resolved Hide resolved
src/earthdataVarinfo/handler.py Outdated Show resolved Hide resolved
src/earthdataVarinfo/handler.py Outdated Show resolved Hide resolved
src/earthdataVarinfo/handler.py Show resolved Hide resolved
@abbottry
Copy link
Member

@owenlittlejohns we absolutely need python tests, you're right! We should also tie that into GitHub actions.

@william-valencia
Copy link
Contributor Author

Added a python unit test.
From src/earthdataVarinfo
Need to run
python3 -m unittest discover
...

Ran 3 tests in 0.023s

OK

Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

Thanks for adding the unit tests. I think the only thing missing from them is plugging them in to the CI/CD workflow so that they run for PRs, etc. (Unless I've missed something here)

src/earthdataVarinfo/test/test_handler.py Show resolved Hide resolved
README.md Outdated

Choose a reason for hiding this comment

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

As a person who is new to GraphQL is it possible we could have something that links to a "NASA How To for GraphQL". This ReadMe feels a bit dense. I can't tell what I need to install and how to get there and what part is actually related to earthdata-varinfo. It seems like the section for "Generate Collection Variable Drafts", is what is applicable here but it doesn't come off really clearly to me.

Copy link

@eni-awowale eni-awowale left a comment

Choose a reason for hiding this comment

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

I think most of this looks good. I didn't really give too much feedback on the GraphQL portions because I am not too familiar with it.

src/earthdataVarinfo/handler.py Show resolved Hide resolved
src/earthdataVarinfo/handler.py Show resolved Hide resolved
Copy link
Member

@owenlittlejohns owenlittlejohns left a comment

Choose a reason for hiding this comment

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

Thanks for plumbing in the tests to the CI/CD. Nice work!

Copy link
Contributor

@macrouch macrouch left a comment

Choose a reason for hiding this comment

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

Only requesting changes to block merging, so I can do a test deployment

william-valencia and others added 26 commits October 17, 2023 14:11
* Switch to using the AWS SDK instead of HTTP requests

* DAS-1899: Fixes a typo

* DAS-1899: Fixed jest tests

* DAS-1899: Renaming files with lowercase letters

---------

Co-authored-by: William Valencia <william.m.valencia@nasa.gov>
@macrouch macrouch merged commit 536c381 into main Oct 17, 2023
6 checks passed
@macrouch macrouch deleted the DAS-1899 branch October 17, 2023 18:41
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.

6 participants