feat(dynamo db): integrate with dynamo db #770

kishore03109 · 2023-05-18T03:25:55Z

Problem

This is the first PR in the effort to remove the usage of queues in our backend.

Doesn't fully close IS-186, but am putting a feature flag in the spirit of merging some work into prod first.

Solution

The flag ARE_QUEUES_DEPRECIATED in InfraService.ts will determine whether or not to use the existing queues or use the dynamodbs vs queues for the site launch process. Idea is after the integration work in the infra side + enough testing, we can swap swap the value of the flag for one site launch process, and then if there are no issues for a while in production, we can remove this feature flag + queues related code altogether.

Breaking Changes

Yes - this PR contains breaking changes
- Details ...
No - this PR is backwards compatible

Reviewer Notes
I am not sure if its only me but the typing given in the AWS SDK for dynamo DB is abit off ._.
I have given a screenshot in the related comment below + added test cases to explicitly codify the expected behaviour that we expect from the dynamoDBDocClient.send(new ScanCommand(params)) call. Open to feedback on this.

Steps to replicate the error thrown by ts

Add this code in server.js:

import DynamoDBService from "@root/services/infra/DynamoDBService"
const dynamoDbService = new DynamoDBService(new DynamoDBDocClient())
const dynamoDBTests = async () => {
  // put item in dynamoDb
  const createResult = await dynamoDbService.createItem({
    repoName: "my-repo",
    appId: "my-app",
    primaryDomainSource: "example.com",
    primaryDomainTarget: "myapp.example.com",
    domainValidationSource: "example.com",
    domainValidationTarget: "myapp.example.com",
    requestorEmail: "[email protected]",
    agencyEmail: "[email protected]",
    githubRedirectionUrl: "https://github.com/my-repo",
    redirectionDomain: [
      {
        source: "example.com",
        target: "myapp.example.com",
        type: "A",
      },
    ],
    status: { state: "pending", message: "PENDING_DURING_SITE_LAUNCH" },
  })

  const updateResult = await dynamoDbService.getAllCompletedLaunches()
  console.log("result: ", updateResult.$metadata.httpStatusCode)
  // delete item from dynamoDB
}
dynamoDBTests()
  .then(() => {
    logger.info("Successfully connected to DynamoDB")
  })
  .catch((err) => {
    logger.error(`Unable to connect to DynamoDB: ${err}`)
  })

Log over the returned entry in DynamoDBService.ts

async getAllCompletedLaunches(): Promise<SiteLaunchMessage[]> {
    const entries = ((await this.dynamoDBClient.getAllItems(this.TABLE_NAME))
      .Items as unknown) as SiteLaunchMessage[]
    const entriesWithSDKTypeCast : Record<string, AttributeValue>[] | undefined= (
      await this.dynamoDBClient.getAllItems(this.TABLE_NAME)
    ).Items

    console.log("entries", entries)
    console.log("entriesWithSDKTypeCast", entriesWithSDKTypeCast)

Note that the returned object is not of the declared type Record<string, AttributeValue>[] | undefined

New environment variables:

STEP_FUNCTIONS_ARN : arn for step functions //todo add in 1pw post-approval of this PR
ARE_QUEUES_DEPRECATED: Whether the queues are deprecated //todo add in 1pw post-approval of this PR

Tests

added a file DynamoDBService.spec.ts for tests

kishore03109 · 2023-05-18T03:38:40Z

src/services/infra/DynamoDBService.ts

-    return this.dynamoDBClient.getItem(this.TABLE_NAME, MOCK_LAUNCH.appId)
-  }
+  async getAllSuccessOrFailureLaunches(): Promise<SiteLaunchMessage[]> {
+    const entries = ((await this.dynamoDBClient.getAllItems(this.TABLE_NAME))


Somehow the typing that the SDK defines the object to have the [DynamoDB types](https://docs.aws.amazon.com/amazondynamodb/latest/developerguide/HowItWorks.NamingRulesDataTypes.html), but the return value already has returned the object of our desired shape O.o Since we are the only ones adding the items to the table, the risk of the shape being different from expected is low. Open to feedback on this !

how do we replicate this behaviour (or if difficult, could you just attach an ss of the stringified Item itself so that we can see in entirety the properties)

Have updated the PR desc to include the steps, but the TLDR when logging the result over is:

For context, we should instead expect an object of a shape something similar to this:

{ "appId": { "S": "my-app" }, "agencyEmail": { "S": "[email protected]" }, "domainValidationSource": { "S": "example.com" }, "domainValidationTarget": { "S": "myapp.example.com" }, "githubRedirectionUrl": { "S": "https://github.com/my-repo" }, "primaryDomainSource": { "S": "example.com" }, "primaryDomainTarget": { "S": "myapp.example.com" }, "PRIMARY_KEY": { "S": "my-app" }, "redirectionDomain": { "L": [ { "M": { "source": { "S": "example.com" }, "target": { "S": "myapp.example.com" }, "type": { "S": "A" } } } ] }, "repoName": { "S": "my-repo" }, "requestorEmail": { "S": "[email protected]" }, "status": { "M": { "message": { "S": "PENDING_DURING_SITE_LAUNCH" }, "state": { "S": "pending" } } } }

src/services/infra/DynamoDBService.ts

seaerchin · 2023-05-19T09:35:15Z

src/services/infra/InfraService.ts

@@ -52,6 +54,7 @@ type CreateSiteParams = {
  isEmailLogin: boolean
 }

+const ARE_QUEUES_DEPRECIATED = false


if this is meant for feature flagging, then it should be an env var. this is because setting this to true still involves a code deployment to our backend (during which we might as well just change the actual code).

changing it to env vars allows us to deploy the required code without an associated code change (ie, we can deploy first and delete later)

Yes that's right. Feature flags modifications shouldn't require a full re-deploy. Hence it shld be in the env vars

Sure, modified to use env vars instead here faed0d0

shall we impose a standard to prefix all feature flags with a FF_ARE_QUEUES_DEPRECATED?

This will allow us to quickly understand an env var is a feature flag toggle

nit: the flag should be something like a verb instead of a question

like DISABLE_SITE_LAUNCH_QUEUES or DEPRECATE_SITE_QUEUES

src/services/infra/DynamoDBService.ts

src/services/infra/DynamoDBClient.ts

src/services/infra/InfraService.ts

microservices/site-launch/shared/types.ts

src/config/config.ts

src/services/infra/InfraService.ts

src/services/infra/DynamoDBService.ts

src/services/infra/InfraService.ts

harishv7 · 2023-05-24T02:33:59Z

src/services/infra/InfraService.ts

@@ -387,7 +404,9 @@ export default class InfraService {
  }

  siteUpdate = async () => {
-    const messages = await this.queueService.pollMessages()
+    const messages = DEPRECATE_SITE_QUEUES
+      ? await this.dynamoDBService.getAllCompletedLaunches()


if any of this throws, are the errors caught appropriately?

using a trivial try-catch wrapper here. Understand that we have an error handling portion to standardise the error formatting, will modify that when the scaffolding work for the error formatting is up! 0fa7912

harishv7 · 2023-05-24T02:36:54Z

src/services/infra/StepFunctionsService.ts

+  private client: StepFunctions
+
+  constructor(private stateMachineArn: string) {
+    this.client = new StepFunctions({ region: "ap-southeast-1" })


should the region come from env vars?

yes, modified here ca073d1

@harishv7 out of curiosity - why inject it from env vars? my understanding is that it's a constant (as we aren't gonna change region) so i'd be more inclined to export it from a constants file

@seaerchin I am fine with a constants file too. I suggested env vars, because we are building this open source, for people to try this, they would need to know exactly what are the config they need to provide. Which would be better to come from an env-var or an explicit config.js file

kishore03109 · 2023-05-24T06:19:18Z

@seaerchin Hey, let me know if you have any concerns regarding this PR via slack, happy to discuss / create new tickets if needed!

kishore03109 commented May 18, 2023

View reviewed changes

kishore03109 marked this pull request as ready for review May 18, 2023 03:40

kishore03109 requested review from harishv7, alexanderleegs, seaerchin and QiluXie May 18, 2023 03:40

alexanderleegs force-pushed the develop branch from ae3f22f to c1363db Compare May 19, 2023 01:31

kishore03109 added 6 commits May 19, 2023 15:29

feat(step functions service): add step functions service

f89ca5c

feat(dynamo cb client): add get all items function

506895d

feat(config): add env var for step functions

61e3781

feat(dynamodb): integration with infra service

8b805be

test(dynamodb service): add test cases

f6bbace

fix(infra service): fix wrong feature flag

b109eed

kishore03109 force-pushed the IS-186-Backend-Add-in-integration-with-site-launch-and-dynamo-DB branch from 7acaebe to b109eed Compare May 19, 2023 07:29

seaerchin reviewed May 19, 2023

View reviewed changes

src/services/infra/DynamoDBService.ts Outdated Show resolved Hide resolved

seaerchin reviewed May 19, 2023

View reviewed changes

src/services/infra/DynamoDBService.ts Outdated Show resolved Hide resolved

seaerchin reviewed May 19, 2023

View reviewed changes

src/services/infra/DynamoDBService.ts Outdated Show resolved Hide resolved

seaerchin reviewed May 19, 2023

View reviewed changes

src/services/infra/DynamoDBService.ts Outdated Show resolved Hide resolved

harishv7 reviewed May 19, 2023

View reviewed changes

src/services/infra/DynamoDBClient.ts Show resolved Hide resolved

harishv7 reviewed May 19, 2023

View reviewed changes

src/services/infra/InfraService.ts Outdated Show resolved Hide resolved

kishore03109 added 2 commits May 22, 2023 09:07

style(infra service): use env var rather than inline const

faed0d0

feat(dynamodb service): add type guard

5391e60

kishore03109 requested review from harishv7 and seaerchin May 22, 2023 02:04

kishore03109 added 2 commits May 22, 2023 10:32

fix(typeguard): fix type guard

253ecfb

style(env var): change naming

0b0dbb1

seaerchin reviewed May 22, 2023

View reviewed changes

kishore03109 added 2 commits May 22, 2023 15:49

fix(typeguard): check for undefined values

fe1a830

style(env var): better naming

0694a08

kishore03109 requested a review from seaerchin May 22, 2023 08:18

fix(config): give default value for step functions arn

90b36c5

mergify bot mentioned this pull request May 23, 2023

Is 185 backend add in integrations with site launch and step functions #775

Closed

harishv7 reviewed May 24, 2023

View reviewed changes

kishore03109 added 4 commits May 24, 2023 10:58

style(dynamoDb service): add args to constructor for ease of testing

5af0826

fix(infra service): add awaits

bbe1d9c

fix(step functions): region should come from env vars

ca073d1

fix(infra service): handle error appropriately

0fa7912

kishore03109 requested a review from harishv7 May 24, 2023 03:15

harishv7 approved these changes May 24, 2023

View reviewed changes

kishore03109 merged commit b768fea into develop May 24, 2023

mergify bot deleted the IS-186-Backend-Add-in-integration-with-site-launch-and-dynamo-DB branch May 24, 2023 06:19

This was referenced May 25, 2023

release(0.28.0): merge to develop #780

Merged

release(0.28.0): merge to prod #781

Merged

liangyuanruo mentioned this pull request Jun 27, 2023

[Snyk] Fix for 6 vulnerabilities #816

Merged

liangyuanruo mentioned this pull request Jun 22, 2023

[Snyk] Security upgrade jsonwebtoken from 8.5.1 to 9.0.0 #597

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dynamo db): integrate with dynamo db #770

feat(dynamo db): integrate with dynamo db #770

kishore03109 commented May 18, 2023 •

edited

Loading

kishore03109 May 18, 2023

seaerchin May 19, 2023 •

edited

Loading

kishore03109 May 22, 2023

seaerchin May 19, 2023

harishv7 May 19, 2023

kishore03109 May 22, 2023

harishv7 May 22, 2023

harishv7 May 22, 2023

kishore03109 May 22, 2023

harishv7 May 24, 2023

kishore03109 May 24, 2023

harishv7 May 24, 2023

kishore03109 May 24, 2023

seaerchin May 24, 2023

harishv7 May 24, 2023

kishore03109 commented May 24, 2023

feat(dynamo db): integrate with dynamo db #770

feat(dynamo db): integrate with dynamo db #770

Conversation

kishore03109 commented May 18, 2023 • edited Loading

Problem

Solution

Choose a reason for hiding this comment

seaerchin May 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishore03109 commented May 24, 2023

kishore03109 commented May 18, 2023 •

edited

Loading

seaerchin May 19, 2023 •

edited

Loading