From 13c40641e7fa50b5165cd4ac69641d787af3c150 Mon Sep 17 00:00:00 2001 From: tyler Date: Thu, 21 Dec 2023 01:41:33 +0000 Subject: [PATCH 1/9] Ensure ESLint checks run for api and web sides --- .github/workflows/qa.yml | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/.github/workflows/qa.yml b/.github/workflows/qa.yml index 0abe4f7b..762f6c76 100644 --- a/.github/workflows/qa.yml +++ b/.github/workflows/qa.yml @@ -167,12 +167,10 @@ jobs: env: CI: 1 - name: Run linter for api side - continue-on-error: true - run: | - yarn eslint api/src + run: yarn eslint api/src - name: Run linter for web side - run: | - yarn eslint web/src + if: ${{ !cancelled() }} + run: yarn eslint web/src tflint: name: Lint terraform From 6a29eb29c699053b976db45c89c2a478b7eda934 Mon Sep 17 00:00:00 2001 From: tyler Date: Thu, 21 Dec 2023 01:43:39 +0000 Subject: [PATCH 2/9] Ensure TFLint plugins are cached --- .github/workflows/qa.yml | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/.github/workflows/qa.yml b/.github/workflows/qa.yml index 762f6c76..f71344ec 100644 --- a/.github/workflows/qa.yml +++ b/.github/workflows/qa.yml @@ -188,7 +188,7 @@ jobs: - uses: actions/cache@704facf57e6136b1bc63b828d79edcd491f0ee84 # v3.3.2 name: Cache plugin dir with: - path: .tflint.d/plugins + path: ~/.tflint.d/plugins key: ${{ runner.os }}-tflint-${{ hashFiles('terraform/.tflint.hcl') }} - uses: terraform-linters/setup-tflint@19a52fbac37dacb22a09518e4ef6ee234f2d4987 # v4.0.0 name: Setup TFLint @@ -203,4 +203,5 @@ jobs: env: GITHUB_TOKEN: ${{ github.token }} - name: Run TFLint + working-directory: terraform run: tflint --format compact --recursive --minimum-failure-severity=error From 1644911a960806a845d2c42450855939aa877fbe Mon Sep 17 00:00:00 2001 From: tyler Date: Thu, 21 Dec 2023 01:48:00 +0000 Subject: [PATCH 3/9] Require `pr-number` input for QA workflow --- .github/workflows/publish-qa-results.yml | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/.github/workflows/publish-qa-results.yml b/.github/workflows/publish-qa-results.yml index e276ea43..84bb417a 100644 --- a/.github/workflows/publish-qa-results.yml +++ b/.github/workflows/publish-qa-results.yml @@ -23,7 +23,7 @@ on: required: true pr-number: type: string - required: false + required: true write-summary: type: boolean default: true @@ -101,19 +101,16 @@ jobs: echo "REPORT_CONTENT<> $GITHUB_OUTPUT echo "$CONTENT" >> $GITHUB_OUTPUT echo "ENDOFREPORT" >> $GITHUB_OUTPUT - - name: Warn on missing comment requirements - if: inputs.write-comment && inputs.pr-number == '' - run: "echo 'WARNING: Cannot write a comment because pr-number is not set'" - name: Find previous report comment id: find-comment - if: inputs.write-comment && inputs.pr-number != '' + if: inputs.write-comment uses: peter-evans/find-comment@a54c31d7fa095754bfef525c0c8e5e5674c4b4b1 # v2.4.0 with: issue-number: ${{ inputs.pr-number }} comment-author: 'github-actions[bot]' body-includes: QA Summary - name: Create or update comment - if: inputs.write-comment && inputs.pr-number != '' + if: inputs.write-comment uses: peter-evans/create-or-update-comment@23ff15729ef2fc348714a3bb66d2f655ca9066f2 # v3.1.0 with: comment-id: ${{ steps.find-comment.outputs.comment-id }} From d8e14ab77e3ed0472e2be0859f29890853afe0fd Mon Sep 17 00:00:00 2001 From: tyler Date: Thu, 21 Dec 2023 01:49:01 +0000 Subject: [PATCH 4/9] chore: version bump tflint aws plugin --- terraform/.tflint.hcl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/terraform/.tflint.hcl b/terraform/.tflint.hcl index 29f8df66..c802daf8 100644 --- a/terraform/.tflint.hcl +++ b/terraform/.tflint.hcl @@ -5,6 +5,6 @@ plugin "terraform" { plugin "aws" { enabled = true - version = "0.22.1" + version = "0.28.0" source = "github.com/terraform-linters/tflint-ruleset-aws" } From fe73cee930d3933caf799b52232e356084c27b6a Mon Sep 17 00:00:00 2001 From: tyler Date: Thu, 21 Dec 2023 01:50:15 +0000 Subject: [PATCH 5/9] ESLint auto-fix --- api/src/lib/aws.ts | 94 ++++++++++++++++++++++++++++------------------ 1 file changed, 57 insertions(+), 37 deletions(-) diff --git a/api/src/lib/aws.ts b/api/src/lib/aws.ts index bb619a5b..5abc5af5 100644 --- a/api/src/lib/aws.ts +++ b/api/src/lib/aws.ts @@ -4,11 +4,15 @@ import { PutObjectCommandInput, S3Client, } from '@aws-sdk/client-s3' -import {ReceiveMessageCommand, SendMessageCommand, SQSClient} from '@aws-sdk/client-sqs' -import {getSignedUrl as awsGetSignedUrl} from '@aws-sdk/s3-request-presigner' +import { + ReceiveMessageCommand, + SendMessageCommand, + SQSClient, +} from '@aws-sdk/client-sqs' +import { getSignedUrl as awsGetSignedUrl } from '@aws-sdk/s3-request-presigner' function getS3Client() { - let s3: S3Client; + let s3: S3Client if (process.env.LOCALSTACK_HOSTNAME) { /* 1. Make sure the local environment has awslocal installed. @@ -18,38 +22,44 @@ function getS3Client() { - awslocal s3api list-buckets - awslocal s3api list-objects --bucket arpa-audit-reports */ - console.log('------------ USING LOCALSTACK ------------'); - const endpoint = `http://${process.env.LOCALSTACK_HOSTNAME}:${process.env.EDGE_PORT || 4566}`; - console.log(`endpoint: ${endpoint}`); + console.log('------------ USING LOCALSTACK ------------') + const endpoint = `http://${process.env.LOCALSTACK_HOSTNAME}:${ + process.env.EDGE_PORT || 4566 + }` + console.log(`endpoint: ${endpoint}`) s3 = new S3Client({ endpoint, forcePathStyle: true, region: process.env.AWS_DEFAULT_REGION, - }); + }) } else { - s3 = new S3Client(); + s3 = new S3Client() } - return s3; + return s3 } -async function sendPutObjectToS3Bucket(bucketName: string, key: string, body: any) { - const s3 = getS3Client(); - const uploadParams : PutObjectCommandInput = { +async function sendPutObjectToS3Bucket( + bucketName: string, + key: string, + body: any +) { + const s3 = getS3Client() + const uploadParams: PutObjectCommandInput = { Bucket: bucketName, Key: key, Body: body, ServerSideEncryption: 'AES256', - }; - await s3.send(new PutObjectCommand(uploadParams)); + } + await s3.send(new PutObjectCommand(uploadParams)) } async function sendHeadObjectToS3Bucket(bucketName: string, key: string) { - const s3 = getS3Client(); - const uploadParams : PutObjectCommandInput = { + const s3 = getS3Client() + const uploadParams: PutObjectCommandInput = { Bucket: bucketName, Key: key, - }; - await s3.send(new PutObjectCommand(uploadParams)); + } + await s3.send(new PutObjectCommand(uploadParams)) } /** @@ -57,33 +67,39 @@ async function sendHeadObjectToS3Bucket(bucketName: string, key: string) { * Exists to organize the imports and to make it easier to mock in tests. */ async function getSignedUrl(bucketName: string, key: string) { - const s3 = getS3Client(); - const baseParams = { Bucket: bucketName, Key: key }; - return awsGetSignedUrl(s3, new GetObjectCommand(baseParams), { expiresIn: 60 }); + const s3 = getS3Client() + const baseParams = { Bucket: bucketName, Key: key } + return awsGetSignedUrl(s3, new GetObjectCommand(baseParams), { + expiresIn: 60, + }) } function getSQSClient() { - let sqs: SQSClient; + let sqs: SQSClient if (process.env.LOCALSTACK_HOSTNAME) { - console.log('------------ USING LOCALSTACK FOR SQS ------------'); - const endpoint = `http://${process.env.LOCALSTACK_HOSTNAME}:${process.env.EDGE_PORT || 4566}`; - sqs = new SQSClient({ endpoint, region: process.env.AWS_DEFAULT_REGION }); + console.log('------------ USING LOCALSTACK FOR SQS ------------') + const endpoint = `http://${process.env.LOCALSTACK_HOSTNAME}:${ + process.env.EDGE_PORT || 4566 + }` + sqs = new SQSClient({ endpoint, region: process.env.AWS_DEFAULT_REGION }) } else { - sqs = new SQSClient(); + sqs = new SQSClient() } - return sqs; + return sqs } async function sendSqsMessage(queueUrl: string, messageBody: any) { - const sqs = getSQSClient(); - await sqs.send(new SendMessageCommand({ - QueueUrl: queueUrl, - MessageBody: JSON.stringify(messageBody), - })); + const sqs = getSQSClient() + await sqs.send( + new SendMessageCommand({ + QueueUrl: queueUrl, + MessageBody: JSON.stringify(messageBody), + }) + ) } async function receiveSqsMessage(queueUrl: string) { - const sqs = getSQSClient(); + const sqs = getSQSClient() // const receiveResp = await sqs.send(new ReceiveMessageCommand({ // QueueUrl: process.env.TASK_QUEUE_URL, WaitTimeSeconds: 20, MaxNumberOfMessages: 1, // })); @@ -92,9 +108,13 @@ async function receiveSqsMessage(queueUrl: string) { // QueueUrl: process.env.TASK_QUEUE_URL, WaitTimeSeconds: 20, MaxNumberOfMessages: 1, // })); - await sqs.send(new ReceiveMessageCommand({ - QueueUrl: queueUrl, WaitTimeSeconds: 20, MaxNumberOfMessages: 1, - })); + await sqs.send( + new ReceiveMessageCommand({ + QueueUrl: queueUrl, + WaitTimeSeconds: 20, + MaxNumberOfMessages: 1, + }) + ) } export default { @@ -103,4 +123,4 @@ export default { getSignedUrl, sendSqsMessage, receiveSqsMessage, -}; +} From 177478cc6b7f808c3fb9f797cc43576d4613c758 Mon Sep 17 00:00:00 2001 From: tyler Date: Thu, 21 Dec 2023 01:52:43 +0000 Subject: [PATCH 6/9] Add documentation for troubleshooting QA checks --- .github/workflows/publish-qa-results.yml | 2 + docs/resolving-qa-failures.md | 66 ++++++++++++++++++++++++ 2 files changed, 68 insertions(+) create mode 100644 docs/resolving-qa-failures.md diff --git a/.github/workflows/publish-qa-results.yml b/.github/workflows/publish-qa-results.yml index 84bb417a..e5f8ead6 100644 --- a/.github/workflows/publish-qa-results.yml +++ b/.github/workflows/publish-qa-results.yml @@ -54,6 +54,8 @@ jobs: cat >> $REPORT_FILE << 'ENDOFREPORT' ## QA Summary + _[See our documentation for tips on how to resolve failing QA checks.](${{ env.GH_SERVER}}/${{ env.GH_REPO }}/blob/main/docs/resolving-qa-failures.md)_ + | QA Check | Result | |:----------------|:-------:| | 🌐 Web Tests | ${{ (env.WEB_TEST_OUTCOME == 'success' && '✅') || (env.WEB_TEST_OUTCOME == 'skipped' && '➖') || '❌' }} | diff --git a/docs/resolving-qa-failures.md b/docs/resolving-qa-failures.md new file mode 100644 index 00000000..ee7acac6 --- /dev/null +++ b/docs/resolving-qa-failures.md @@ -0,0 +1,66 @@ +# Tips for Resolving QA Check Failures + +The QA checks that run against open pull requests exist to help ensure that changes submitted in PRs are working properly and are consistent with our adopted best practices. +If you have encountered a QA check failure and are unsure of how to resolve it, then this guide is for you! + +As always, you can reach out to the Grants team for help if you are still not sure about the best way to resolve a particular problem. + +## Linting Failures + +> [!TIP] +> All ESLint and TFLint findings will result in a check annotation on the relevent line(s) in your pull request's code. +> You can review them from the "Files changed" view for your PR. + +### ESLint + +The [ESLint](https://eslint.org/) QA check determines whether JavaScript and/or TypeScript files beneath the `api/src` and `web/src` directories are formatted consistently with the rules configured for this project. +Some issues that ESLint reports are just warnings; we recommend that you fix these if you can, +but they will not prevent you from merging your pull request (and they do cause the QA check to fail). +Other ESLint findings are considered errors and are requirements for merging your pull request. + +To fix issues reported by ESLint, first review the failures by running `yarn eslint api/src web/src` from the root of this repository, which will output all warnings and errors. +Many ESLint issues can be fixed automatically by adding the `--fix` flag to this command, but you can also resolve them manually if you prefer. + +Sometimes, an ESLint finding is not valid for a particular reason - +maybe you need a nonstandard import behavior, or an import is necessary for its side-effects. +In these cases, specific findings can be ignored. +Check the [ESLint documentation](https://eslint.org/docs/latest/use/configure/rules) for the most appropriate way to ignore the finding. + +### TFlint + +The [TFLint](https://github.com/terraform-linters/tflint) QA check determines whether Terraform files beneath the `terraform/` directory are using best practices and/or contain potential errors. +These checks can often help address problems that can cause a failure during `terraform apply` +(but may not be found during `terraform plan`), +which makes it especially useful for avoiding failed deployments after a PR is merged. + +To run TFlint locally, you might need to [install](https://github.com/terraform-linters/tflint?tab=readme-ov-file#installation) it first. +Once installed, navigate to the `terraform/` directory of the repository and run `tflint --init` +to ensure that the plugins configured in the [`.tflint.hcl` file](https://github.com/usdigitalresponse/cpf-reporter/blob/main/terraform/.tflint.hcl) are up-to-date. Then, run the linter checks by running +`tflint --recursive --minimum-failure-severity=error` to view the problems. +Many TFLint issues can be fixed automatically by adding the `--fix` flag to this command, but you can also resolve them manually if you prefer. + +Sometimes, a TFLint finding is not valid for a particular reason. +Or, despite being valid, the team has agreed that it may be bypassed after confirming that doing so will not cause an immediate problem. +In these cases, a `tflint-ignore` pragma comment may be used as an annotation in order to avoid a specific rule violation, +or a `rule` block may be added to `terraform/.tflint.hcl` to ignore the linting rule entirely. +Most TFlint errors will include a reference link that provides justification for the rule's importance and ways to resolve. + +For example, if TFLint fails with an error like this: + +``` +Warning: variable "not_used" is declared but not used (terraform_unused_declarations) + + on config.tf line 1: + 1: variable "not_used" { + +Reference: https://github.com/terraform-linters/tflint-ruleset-terraform/blob/v0.1.0/docs/rules/terraform_unused_declarations.md +``` + +the `terraform_unused_declarations` rule can be disabled only for the `not_used` variable by adding a pragma comment above the variable declaration: +```terraform +# tflint-ignore: terraform_unused_declarations +variable "not_used" { + type = string + description = "This variable is never used." +} +``` From 55e2d1eedf8c153ba2edc001577534803a9630b8 Mon Sep 17 00:00:00 2001 From: tyler Date: Thu, 21 Dec 2023 02:22:10 +0000 Subject: [PATCH 7/9] Add type for S3 PutObject body value --- api/src/lib/aws.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/api/src/lib/aws.ts b/api/src/lib/aws.ts index 5abc5af5..22b6f5e4 100644 --- a/api/src/lib/aws.ts +++ b/api/src/lib/aws.ts @@ -10,6 +10,7 @@ import { SQSClient, } from '@aws-sdk/client-sqs' import { getSignedUrl as awsGetSignedUrl } from '@aws-sdk/s3-request-presigner' +import { StreamingBlobPayloadInputTypes } from '@smithy/types' function getS3Client() { let s3: S3Client @@ -41,8 +42,8 @@ function getS3Client() { async function sendPutObjectToS3Bucket( bucketName: string, key: string, - body: any -) { + body: StreamingBlobPayloadInputTypes +): Promise { const s3 = getS3Client() const uploadParams: PutObjectCommandInput = { Bucket: bucketName, From 9d9e287cfd180bdd6ba626d597119b835ec9be41 Mon Sep 17 00:00:00 2001 From: tyler Date: Thu, 21 Dec 2023 02:22:34 +0000 Subject: [PATCH 8/9] Resolve eslint no-explicit-any --- api/src/lib/aws.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/api/src/lib/aws.ts b/api/src/lib/aws.ts index 22b6f5e4..96105448 100644 --- a/api/src/lib/aws.ts +++ b/api/src/lib/aws.ts @@ -89,7 +89,7 @@ function getSQSClient() { return sqs } -async function sendSqsMessage(queueUrl: string, messageBody: any) { +async function sendSqsMessage(queueUrl: string, messageBody: unknown) { const sqs = getSQSClient() await sqs.send( new SendMessageCommand({ From b925252076cf41637fbcbdf879814a12570f2945 Mon Sep 17 00:00:00 2001 From: tyler Date: Fri, 5 Jan 2024 21:42:07 +0000 Subject: [PATCH 9/9] Remove unneeded eslint pragma comments --- api/src/lib/aws.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/api/src/lib/aws.ts b/api/src/lib/aws.ts index fa32f5d4..94948f63 100644 --- a/api/src/lib/aws.ts +++ b/api/src/lib/aws.ts @@ -116,12 +116,11 @@ export async function s3PutSignedUrl( }) return url } + /** * This function is a wrapper around the getSignedUrl function from the @aws-sdk/s3-request-presigner package. * Exists to organize the imports and to make it easier to mock in tests. */ - -// eslint-disable-next-line @typescript-eslint/no-unused-vars async function getSignedUrl(bucketName: string, key: string) { const s3 = getS3Client() const baseParams = { Bucket: bucketName, Key: key } @@ -144,7 +143,6 @@ function getSQSClient() { return sqs } -// eslint-disable-next-line @typescript-eslint/no-unused-vars async function sendSqsMessage(queueUrl: string, messageBody: unknown) { const sqs = getSQSClient() await sqs.send( @@ -155,7 +153,6 @@ async function sendSqsMessage(queueUrl: string, messageBody: unknown) { ) } -// eslint-disable-next-line @typescript-eslint/no-unused-vars async function receiveSqsMessage(queueUrl: string) { const sqs = getSQSClient() // const receiveResp = await sqs.send(new ReceiveMessageCommand({