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

storing config in dynamodb table poc #12

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

danieljse
Copy link

@danieljse danieljse commented Mar 17, 2023

https://github.com/opentorc/torc-serverless/issues/557

This PR is a POC for storing config parameters in Config table - related PR https://github.com/opentorc/platform-api/pull/1133 - added two methods to the cli to be able to test it.

  1. Create Config table mentioned in the related PR
    1.a Creat SSM variable DYNAMODB_CONFIG_TABLE under common path

  2. install config-wrapper package locally npm i path../.../config-wrapper

  3. create sharedConfig.json
    eg

{
  "DYNAMODB_USER_TABLE_STREAM": {
      "value": "TABLE_NAME"
  },
  "ASSESSMENT_CONFIG": {
     "value":  {
              "testId": 123459,
              "valir": false
     }
  },
  "ARRAY_CONFIG" : {
    "value": [{
      "testValue" : 5
    }]
  }
}
  1. with the cli create service parameters config-wrapper putSharedConfigFromFile --infile sharedConfig.json --env develop --service resolvers

  2. with the cli get service parameters config-wrapper getSharedConfigByService --env develop --service resolvers

—-

NOTE: When configWrapper.awsManager.getSharedConfigByService() method is called in a lambda function. Lambda function should have dynamodb:Query permission for the Config_Table including Config_Table/index/*

src/cli.js Outdated
type: 'input',
name: 'env',
message: 'Environment: ',
default: 'devevelop'
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo. should be develop

src/lib/awsManager.js Outdated Show resolved Hide resolved
src/lib/awsManager.js Outdated Show resolved Hide resolved
src/lib/awsManager.js Outdated Show resolved Hide resolved
src/lib/awsManager.js Outdated Show resolved Hide resolved
src/lib/awsManager.js Show resolved Hide resolved
src/cli.js Show resolved Hide resolved
setParameter,
setParametersByService,
getEnvironments,
getServicesForEnvironment,
getAllOrgParams
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You would need to update README file too - with the 2 new commands

Copy link
Contributor

Choose a reason for hiding this comment

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

This bit is still pending

Copy link
Author

Choose a reason for hiding this comment

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

updated thanks.

src/lib/awsManager.js Show resolved Hide resolved
tests/index.js Outdated
should.exist(found)
found.value.should.equal(getSharedConfigParams[i].value)
}
})
Copy link
Contributor

Choose a reason for hiding this comment

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

Test is same as the one before - to test if cached value returned, you would need to first retrieve the values, then update them - and then retrieve them again to verify that you are NOT getting the updated value... thus proving that it is indeed read from cache.

However, you don't have to write such a test right now. So you can remove this one.

@callmekatootie
Copy link
Contributor

Great work on this btw!

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.

3 participants