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

Ingest forecasted opportunities #848

Merged
merged 10 commits into from
Jul 10, 2024
Merged

Ingest forecasted opportunities #848

merged 10 commits into from
Jul 10, 2024

Conversation

jeffsmohan
Copy link
Contributor

@jeffsmohan jeffsmohan commented Jun 14, 2024

Ticket #330

Description

This PR refactors the SplitGrantsGovXMLDB lambda function so it ingests both grant opportunities and forecasted grants.

As the lambda is scanning through the extracted grants.gov XML, it will now identify both OpportunitySynopsisDetail_1_0 and OpportunityForecastDetail_1_0 XML tags (whereas it previously only identified the former). These individual XML records will still be stashed as individual S3 files, but now with updated filenames to distinguish forecasted vs not.

For now, we're also updating the filtering for the downstream PersistGrantsGovXMLDB lambda, so it only picks up on opportunities (not forecasted). Future work will enable forecasted grants through the rest of the pipeline.

Testing

  • Added/updated unit tests to ensure it processes forecasted grants
  • I'd like to verify in awslocal that the lambda runs and stashes the expected S3 objects for forecasted grants, but I'm struggling to get that working properly... I'll try to work on that with ty before merging

Automated and Unit Tests

  • Added Unit tests

Manual tests for Reviewer

  • Added steps to test feature/functionality manually

Checklist

  • Provided ticket and description
  • Provided testing information
  • Provided adequate test coverage for all new code
  • Added PR reviewers

@github-actions github-actions bot added go Pull requests that update Go code terraform Pull requests that update Terraform code labels Jun 14, 2024
Copy link

github-actions bot commented Jun 14, 2024

Terraform Summary

Step Result
🖌 Terraform Format & Style
⚙️ Terraform Initialization
🤖 Terraform Validation
📖 Terraform Plan

Output

Validation Output
Success! The configuration is valid.


Plan Output
Terraform used the selected providers to generate the following execution
plan. Resource actions are indicated with the following symbols:
  ~ update in-place
-/+ destroy and then create replacement

Terraform will perform the following actions:

  # aws_s3_bucket_notification.grant_prepared_data will be updated in-place
  ~ resource "aws_s3_bucket_notification" "grant_prepared_data" {
        id          = "grantsingest-grantsprepareddata-357150818708-us-west-2"
        # (2 unchanged attributes hidden)

      ~ lambda_function {
          ~ filter_suffix       = "/ffis.org/v1.json" -> "/grants.gov/v2.OpportunitySynopsisDetail_1_0.xml"
            id                  = "tf-s3-lambda-20230826035241633000000017"
          ~ lambda_function_arn = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PersistFFISData" -> "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PersistGrantsGovXMLDB"
            # (1 unchanged attribute hidden)
        }
+       lambda_function {
+           events              = [
+               "s3:ObjectCreated:*",
            ]
+           filter_suffix       = "/ffis.org/v1.json"
+           lambda_function_arn = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PersistFFISData"
        }

        # (1 unchanged block hidden)
    }

  # module.DownloadFFISSpreadsheet.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-DownloadFFISSpreadsheet"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-DownloadFFISSpreadsheet:54" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-DownloadFFISSpreadsheet:54/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "54" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                      = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:downloadffisspreadsheet" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:downloadffisspreadsheet"
              ~ "DD_VERSION"                   = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (11 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.DownloadFFISSpreadsheet.module.lambda_function.aws_lambda_permission.current_version_triggers["SQSQueueNotification"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "SQSQueueNotification" -> (known after apply)
      ~ qualifier           = "54" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (4 unchanged attributes hidden)
    }

  # module.DownloadGrantsGovDB.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-DownloadGrantsGovDB"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-DownloadGrantsGovDB:54" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-DownloadGrantsGovDB:54/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "54" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                        = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:downloadgrantsgovdb" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:downloadgrantsgovdb"
              ~ "DD_VERSION"                     = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (13 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.DownloadGrantsGovDB.module.lambda_function.aws_lambda_permission.current_version_triggers["Schedule"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "Schedule" -> (known after apply)
      ~ qualifier           = "54" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

  # module.EnqueueFFISDownload.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-EnqueueFFISDownload"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-EnqueueFFISDownload:54" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-EnqueueFFISDownload:54/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "54" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                      = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:enqueueffisdownload" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:enqueueffisdownload"
              ~ "DD_VERSION"                   = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (11 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.EnqueueFFISDownload.module.lambda_function.aws_lambda_permission.current_version_triggers["S3BucketNotification"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "S3BucketNotification" -> (known after apply)
      ~ qualifier           = "54" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

  # module.ExtractGrantsGovDBToXML.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-ExtractGrantsGovDBToXML"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-ExtractGrantsGovDBToXML:54" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-ExtractGrantsGovDBToXML:54/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "54" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                      = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:extractgrantsgovdbtoxml" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:extractgrantsgovdbtoxml"
              ~ "DD_VERSION"                   = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (11 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.ExtractGrantsGovDBToXML.module.lambda_function.aws_lambda_permission.current_version_triggers["S3BucketNotification"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "S3BucketNotification" -> (known after apply)
      ~ qualifier           = "54" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

  # module.PersistFFISData.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-PersistFFISData"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PersistFFISData:54" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PersistFFISData:54/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "54" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                       = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:persistffisdata" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:persistffisdata"
              ~ "DD_VERSION"                    = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (11 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.PersistFFISData.module.lambda_function.aws_lambda_permission.current_version_triggers["S3BucketNotification"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "S3BucketNotification" -> (known after apply)
      ~ qualifier           = "54" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

  # module.PersistGrantsGovXMLDB.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-PersistGrantsGovXMLDB"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PersistGrantsGovXMLDB:54" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PersistGrantsGovXMLDB:54/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "54" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                       = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:persistgrantsgovxmldb" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:persistgrantsgovxmldb"
              ~ "DD_VERSION"                    = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (11 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.PersistGrantsGovXMLDB.module.lambda_function.aws_lambda_permission.current_version_triggers["S3BucketNotification"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "S3BucketNotification" -> (known after apply)
      ~ qualifier           = "54" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

  # module.PublishGrantEvents.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-PublishGrantEvents"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PublishGrantEvents:55" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-PublishGrantEvents:55/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "55" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                      = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:publishgrantevents" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:publishgrantevents"
              ~ "DD_VERSION"                   = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (11 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.PublishGrantEvents.module.lambda_function.aws_lambda_permission.current_version_triggers["dynamodb"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "dynamodb" -> (known after apply)
      ~ qualifier           = "55" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

  # module.ReceiveFFISEmail.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-ReceiveFFISEmail"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-ReceiveFFISEmail:53" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-ReceiveFFISEmail:53/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "53" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                        = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:receiveffisemail" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:receiveffisemail"
              ~ "DD_VERSION"                     = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (12 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.ReceiveFFISEmail.module.lambda_function.aws_lambda_permission.current_version_triggers["S3BucketNotification"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "S3BucketNotification" -> (known after apply)
      ~ qualifier           = "53" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

  # module.SplitFFISSpreadsheet.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-SplitFFISSpreadsheet"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-SplitFFISSpreadsheet:54" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-SplitFFISSpreadsheet:54/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "54" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                          = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:splitffisspreadsheet" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:splitffisspreadsheet"
              ~ "DD_VERSION"                       = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
                # (14 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.SplitFFISSpreadsheet.module.lambda_function.aws_lambda_permission.current_version_triggers["S3BucketNotification"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "S3BucketNotification" -> (known after apply)
      ~ qualifier           = "54" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

  # module.SplitGrantsGovXMLDB.module.lambda_function.aws_lambda_function.this[0] will be updated in-place
  ~ resource "aws_lambda_function" "this" {
        id                             = "grants_ingest-SplitGrantsGovXMLDB"
      ~ qualified_arn                  = "arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-SplitGrantsGovXMLDB:54" -> (known after apply)
      ~ qualified_invoke_arn           = "arn:aws:apigateway:us-west-2:lambda:path/2015-03-31/functions/arn:aws:lambda:us-west-2:357150818708:function:grants_ingest-SplitGrantsGovXMLDB:54/invocations" -> (known after apply)
        tags                           = {}
      ~ version                        = "54" -> (known after apply)
        # (21 unchanged attributes hidden)

      ~ environment {
          ~ variables = {
              ~ "DD_TAGS"                          = "git.commit.sha:51dfc5b3a80146af35865f1f8a74a0241120d3b9,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:splitgrantsgovxmldb" -> "git.commit.sha:6f56dc7d579c42a2baffc189eb890f7c4bd4900c,git.repository_url:github.com/usdigitalresponse/grants-ingest,handlername:splitgrantsgovxmldb"
              ~ "DD_VERSION"                       = "51dfc5b3a80146af35865f1f8a74a0241120d3b9" -> "6f56dc7d579c42a2baffc189eb890f7c4bd4900c"
+               "IS_FORECASTED_GRANTS_ENABLED"     = "false"
                # (14 unchanged elements hidden)
            }
        }

        # (3 unchanged blocks hidden)
    }

  # module.SplitGrantsGovXMLDB.module.lambda_function.aws_lambda_permission.current_version_triggers["S3BucketNotification"] must be replaced
-/+ resource "aws_lambda_permission" "current_version_triggers" {
      ~ id                  = "S3BucketNotification" -> (known after apply)
      ~ qualifier           = "54" # forces replacement -> (known after apply) # forces replacement
+       statement_id_prefix = (known after apply)
        # (5 unchanged attributes hidden)
    }

Plan: 10 to add, 11 to change, 10 to destroy.

Pusher: @jeffsmohan, Action: pull_request_target, Workflow: Continuous Integration

@jeffsmohan jeffsmohan marked this pull request as ready for review June 14, 2024 17:33
@jeffsmohan
Copy link
Contributor Author

@TylerHendrickson Thanks for taking a look here! Since this is my first PR in this repo, I've got a few questions/requests hopefully you can help with:

  1. Please feel free to go hard with the comments! I'm not familiar with go idioms and best practices yet, so it's helpful to hear where I may have missed the mark.
  2. I needed to run a few chore/upgrade methods (e.g., go mod tidy) to get things running locally. This seems to have ended up with untracked files like go.mod, go.sum, and terraform/.terraform.lock.hcl. Do you have these in your personal .gitignore? Or is there some other way to ensure these sorts of files don't get checked in, if they're not intended to be?
  3. I was unable to get this fully working in localstack. When I invoked the DownloadGrantsGovDB lambda, it successfully downloaded, and it seems to have triggered SplitGrantsGovXMLDB to run as well. (I was able to run e.g. awslocal s3 ls s3://grants-ingest-grantsprepareddata-000000000000-us-west-1/ and see the ID prefix folders getting created one by one.) But before the process completed, my localstack seems to crash, and all commands return with Connection was closed before we received a valid response from endpoint URL. My guess is I either need to allocate more resources in docker (though fwiw it crashes well before it hits my container memory usage max) or maybe I could temporarily add a filter inside the SplitGrantsGovXMLDB handler to only process a subset of IDs? Are you able to run this locally, or any idea why it's not for me?

@@ -441,7 +441,7 @@ resource "aws_s3_bucket_notification" "grant_prepared_data" {
lambda_function {
lambda_function_arn = module.PersistGrantsGovXMLDB.lambda_function_arn
events = ["s3:ObjectCreated:*"]
filter_suffix = "/grants.gov/v2.xml"
filter_suffix = "/grants.gov/v2.OpportunitySynopsisDetail_1_0.xml"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

For initial deploy, we want to add this as a new lambda_function alongside this existing one (to ensure any in-flight events get handled)

@jeffsmohan
Copy link
Contributor Author

Update on manual testing: I got this working in my localstack by filtering which prefixes to actually save records to S3 for. Literally just added this to the top of the processRecord function in the SplitGrantsGovXMLDB handler:

	if strings.Split(record.s3ObjectKey(), "/")[0] != "284" {
		return nil
	}

And then I was able to verify specific forecast records were being saved to my local S3 in the correct location and with the correct data:

image

Having the two lambdas deployed alongside each other for now assures
we won't drop any events for S3 objects in the old location. Eventually
we can remove the old v2.xml version of the lambda.
Copy link
Member

@TylerHendrickson TylerHendrickson left a comment

Choose a reason for hiding this comment

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

@jeffsmohan The code changes here look excellent!

I do have one comment concerning our strategy for activating these changes in relation to other planned downstream changes. Let me know if the comment makes sense to you and/or you want to discuss further.

Comment on lines +12 to +18
type grantRecord interface {
logWith(log.Logger) log.Logger
// s3ObjectKey returns a string to use as the object key when saving the opportunity to an S3 bucket
s3ObjectKey() string
lastModified() (time.Time, error)
toXML() ([]byte, error)
}
Copy link
Member

Choose a reason for hiding this comment

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

This is great! 👏

Comment on lines 160 to 165
} else if se.Name.Local == GRANT_FORECAST_XML_NAME {
var f forecast
if err = d.DecodeElement(&f, &se); err == nil {
ch <- &f
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we should comment this block or otherwise prevent sending the decoded forecast to the channel until the downstream DynamoDB persistence step is ready to handle OpportunityForecastDetail_1_0.xml object events. As of today, there are just over 600 forecast records in the extract; I expect this to be a similar number once these changes hit Production. If we roll this out as-is, we'll either need to bulk-delete the objects in S3 or otherwise manually trigger the PersistGrantsGovXMLDB step for each fetched object. While I don't think saving the forecast records to S3 ahead of downstream changes would cause any issues in the pipeline, it means that we'd miss out on being able to pull in the bulk of currently-forecasted grants.

Perhaps we should add a feature flag (env var) here that would allow us to still observe that things are working as-expected on Staging, which we could control in Terraform.

Thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, I see... once this runs and the forecasted XML snippets are pushed to S3, they'll be considered extant unless they've been updated for subsequent runs. So when we eventually update the downstream tasks to handle forecasted grants, we won't automatically get the full set of them in the DB. Have I got the concern?

I think this concern points to there being some need for a utility to rebuild the database, which would be useful in this situation and likely in others. (And possibly just a good occasional vacuuming type of task to run periodically.) That's out of scope here; I'm just mentioning it so it's in our heads for the future...

For this PR, it sounds like a flag is the way to go. I'll get on implementing that!

Copy link
Member

Choose a reason for hiding this comment

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

I think this concern points to there being some need for a utility to rebuild the database, which would be useful in this situation and likely in others.

There are a few, let's say "scrappy" ways of doing this today – some equivalent of

aws s3 ls --recursive s3://the-bucket/path | grep 'pattern' | xargs aws lambda invoke ...

I've got some scratch work laying around to turn the contents of cli/ into a more robust maintenance tool for these types of things, but point taken that this should be formalized – good call.

@TylerHendrickson
Copy link
Member

@jeffsmohan re

  1. I needed to run a few chore/upgrade methods (e.g., go mod tidy) to get things running locally. This seems to have ended up with untracked files like go.mod, go.sum, and terraform/.terraform.lock.hcl. Do you have these in your personal .gitignore? Or is there some other way to ensure these sorts of files don't get checked in, if they're not intended to be?

The go.mod and go.sum files are tracked and are used for dependency management. I ran go mod tidy with this branch checked out in a clean workspace and didn't get any resulting changes to those files, so I'm not totally sure why they're getting modified for you, although if you are running a version of Go other than 1.20 and/or you ran something like go get -u, that might explain things. I suggest trying to git checkout -f go.mod go.sum and then running go mod tidy again to see if you get the same result. If you do still get changes to go.mod, would you mind posting the diff here? I'd be curious to see what might be getting detected as an added/removed/re-versioned dependency.

Similarly, I'm not getting any changes to terraform/.terraform.lock.hcl when I run terraform init. Is it possible you ran terraform init with the -upgrade flag? That would explain a dependency update to your lockfile.

@jeffsmohan
Copy link
Contributor Author

jeffsmohan commented Jun 24, 2024

The go.mod and go.sum files are tracked and are used for dependency management. I ran go mod tidy with this branch checked out in a clean workspace and didn't get any resulting changes to those files, so I'm not totally sure why they're getting modified for you, although if you are running a version of Go other than 1.20 and/or you ran something like go get -u, that might explain things. I suggest trying to git checkout -f go.mod go.sum and then running go mod tidy again to see if you get the same result. If you do still get changes to go.mod, would you mind posting the diff here? I'd be curious to see what might be getting detected as an added/removed/re-versioned dependency.

@TylerHendrickson On a clean main branch, when I run go mod tidy I do end up with changes to go.mod and go.sum. They appear to be related to the golang version (e.g., the diff switches go 1.20 to go 1.21 in go.mod). But confusingly, I'm seeing different versions in different places.

  • At the terminal, go version shows go version go1.22.1 darwin/arm64
  • When I run go mod tidy, the diff to go.mod shows go 1.20 changed to go 1.21 but adds toolchain go1.22.1

Why am I seeing 1.21 and 1.22? And I guess more to the point, what are the standard tools for managing multiple versions of go for different projects? How do I best make sure I'm using 1.20 while working on this codebase?

For reference, the full diff after running `go mod tidy`:
diff --git a/go.mod b/go.mod
index f2abb97..b9843cc 100644
--- a/go.mod
+++ b/go.mod
@@ -1,6 +1,8 @@
 module github.com/usdigitalresponse/grants-ingest

-go 1.20
+go 1.21
+
+toolchain go1.22.1

 require (
        github.com/DataDog/datadog-lambda-go v1.17.0
diff --git a/go.sum b/go.sum
index 5a71953..6d284f7 100644
--- a/go.sum
+++ b/go.sum
@@ -1,4 +1,5 @@
 github.com/DATA-DOG/go-sqlmock v1.4.1 h1:ThlnYciV1iM/V0OSF/dtkqWb6xo5qITT1TJBG1MRDJM=
+github.com/DATA-DOG/go-sqlmock v1.4.1/go.mod h1:f/Ixk793poVmq4qj/V1dPUg2JEAKC73Q5eFN3EC/SaM=
 github.com/DataDog/appsec-internal-go v1.5.0 h1:8kS5zSx5T49uZ8dZTdT19QVAvC/B8ByyZdhQKYQWHno=
 github.com/DataDog/appsec-internal-go v1.5.0/go.mod h1:pEp8gjfNLtEOmz+iZqC8bXhu0h4k7NUsW/qiQb34k1U=
 github.com/DataDog/datadog-agent/pkg/obfuscate v0.50.2 h1:y08IzbpFM/HBaKfgayFZe1FpcbZn6bVPXoZ++93vxv8=
@@ -16,6 +17,7 @@ github.com/DataDog/go-sqllexer v0.0.10/go.mod h1:KwkYhpFEVIq+BfobkTC1vfqm4gTi65s
 github.com/DataDog/go-tuf v1.0.2-0.5.2 h1:EeZr937eKAWPxJ26IykAdWA4A0jQXJgkhUjqEI/w7+I=
 github.com/DataDog/go-tuf v1.0.2-0.5.2/go.mod h1:zBcq6f654iVqmkk8n2Cx81E1JnNTMOAx1UEO/wZR+P0=
 github.com/DataDog/gostackparse v0.7.0 h1:i7dLkXHvYzHV308hnkvVGDL3BR4FWl7IsXNPz/IGQh4=
+github.com/DataDog/gostackparse v0.7.0/go.mod h1:lTfqcJKqS9KnXQGnyQMCugq3u1FP6UZMfWR0aitKFMM=
 github.com/DataDog/sketches-go v1.4.4 h1:dF52vzXRFSPOj2IjXSWLvXq3jubL4CI69kwYjJ1w5Z8=
 github.com/DataDog/sketches-go v1.4.4/go.mod h1:XR0ns2RtEEF09mDKXiKZiQg+nfZStrq1ZuL1eezeZe0=
 github.com/Microsoft/go-winio v0.5.0/go.mod h1:JPGBdM1cNvN/6ISo+n8V5iA4v8pBzdOpzfwIujj1a84=
@@ -24,9 +26,11 @@ github.com/Microsoft/go-winio v0.6.1/go.mod h1:LRdKpFKfdobln8UmuiYcKPot9D2v6svN5
 github.com/Netflix/go-env v0.0.0-20220526054621-78278af1949d h1:wvStE9wLpws31NiWUx+38wny1msZ/tm+eL5xmm4Y7So=
 github.com/Netflix/go-env v0.0.0-20220526054621-78278af1949d/go.mod h1:9XMFaCeRyW7fC9XJOWQ+NdAv8VLG7ys7l3x4ozEGLUQ=
 github.com/alecthomas/assert/v2 v2.6.0 h1:o3WJwILtexrEUk3cUVal3oiQY2tfgr/FHWiz/v2n4FU=
+github.com/alecthomas/assert/v2 v2.6.0/go.mod h1:Bze95FyfUr7x34QZrjL+XP+0qgp/zg8yS+TtBj1WA3k=
 github.com/alecthomas/kong v0.9.0 h1:G5diXxc85KvoV2f0ZRVuMsi45IrBgx9zDNGNj165aPA=
 github.com/alecthomas/kong v0.9.0/go.mod h1:Y47y5gKfHp1hDc7CH7OeXgLIpp+Q2m1Ni0L5s3bI8Os=
 github.com/alecthomas/repr v0.4.0 h1:GhI2A8MACjfegCPVq9f1FLvIBS+DrQ2KQBFZP1iFzXc=
+github.com/alecthomas/repr v0.4.0/go.mod h1:Fr0507jx4eOXV7AlPV6AVZLYrLIuIeSOWtW57eE/O/4=
 github.com/andybalholm/brotli v1.1.0 h1:eLKJA0d02Lf0mVpIDgYnqXcUn0GqVmEFny3VuID1U3M=
 github.com/andybalholm/brotli v1.1.0/go.mod h1:sms7XGricyQI9K10gOSf56VKKWS4oLer58Q+mhRPtnY=
 github.com/aws/aws-lambda-go v1.47.0 h1:0H8s0vumYx/YKs4sE7YM0ktwL2eWse+kfopsRI1sXVI=
@@ -63,6 +67,7 @@ github.com/aws/aws-sdk-go-v2/service/dynamodb v1.32.6/go.mod h1:uNhUf9Z3MT6Ex+u0
 github.com/aws/aws-sdk-go-v2/service/dynamodbstreams v1.20.8 h1:PapW7iWHqua6Gk+qRjgXpM3fNqUxY3N+1WURHPcmKhc=
 github.com/aws/aws-sdk-go-v2/service/dynamodbstreams v1.20.8/go.mod h1:IL6qnQxrc/qIjwzeg7USP3P7ySEehOPpXJslRbXNYJ4=
 github.com/aws/aws-sdk-go-v2/service/ec2 v1.93.2 h1:c6a19AjfhEXKlEX63cnlWtSQ4nzENihHZOG0I3wH6BE=
+github.com/aws/aws-sdk-go-v2/service/ec2 v1.93.2/go.mod h1:VX22JN3HQXDtQ3uS4h4TtM+K11vydq58tpHTlsm8TL8=
 github.com/aws/aws-sdk-go-v2/service/eventbridge v1.31.3 h1:72en29uLIOVnNrblHoWavhNxNSKtt3PkPH1+ShhfV0o=
 github.com/aws/aws-sdk-go-v2/service/eventbridge v1.31.3/go.mod h1:H69fMdoeNRj4xalIaWYSpniE3ghC69qaifDnqYiUbP0=
 github.com/aws/aws-sdk-go-v2/service/internal/accept-encoding v1.11.2 h1:Ji0DY1xUsUr3I8cHps0G+XM3WWU16lP6yG8qu1GAZAs=
@@ -114,6 +119,7 @@ github.com/dustin/go-humanize v1.0.1/go.mod h1:Mu1zIs6XwVuF/gI1OepvI0qD18qycQx+m
 github.com/ebitengine/purego v0.6.0-alpha.5 h1:EYID3JOAdmQ4SNZYJHu9V6IqOeRQDBYxqKAg9PyoHFY=
 github.com/ebitengine/purego v0.6.0-alpha.5/go.mod h1:ah1In8AOtksoNK6yk5z1HTJeUkC1Ez4Wk2idgGslMwQ=
 github.com/felixge/httpsnoop v1.0.3 h1:s/nj+GCswXYzN5v2DpNMuMQYe+0DDwt5WVCU6CWBdXk=
+github.com/felixge/httpsnoop v1.0.3/go.mod h1:m8KPJKqk1gH5J9DgRY2ASl2lWCfGKXixSwevea8zH2U=
 github.com/go-kit/log v0.2.1 h1:MRVx0/zhvdseW+Gza6N9rVzU/IVzaeE1SFI4raAhmBU=
 github.com/go-kit/log v0.2.1/go.mod h1:NwTd00d/i8cPZ3xOwwiv2PO5MOcx78fFErGNcVmBjv0=
 github.com/go-logfmt/logfmt v0.6.0 h1:wGYYu3uicYdqXVgoYbvnkrPVXkuLM1p1ifugDMEdRi4=
@@ -129,12 +135,15 @@ github.com/golang/protobuf v1.5.3 h1:KhyjKVUg7Usr/dYsdSqoFveMYd5ko72D+zANwlG1mmg
 github.com/golang/protobuf v1.5.3/go.mod h1:XVQd3VNwM+JqD3oG2Ue2ip4fOMUkwXdXDdiuN0vRsmY=
 github.com/google/go-cmp v0.5.5/go.mod h1:v8dTdLbMG2kIc/vJvl+f65V22dbkXbowE6jgT/gNBxE=
 github.com/google/go-cmp v0.6.0 h1:ofyhxvXcZhMsU5ulbFiLKl/XBFqE1GSq7atu8tAmTRI=
+github.com/google/go-cmp v0.6.0/go.mod h1:17dUlkBOakJ0+DkrSSNjCkIjxS6bF9zb3elmeNGIjoY=
 github.com/google/gofuzz v1.2.0 h1:xRy4A+RhZaiKjJ1bPfwQ8sedCA+YS2YcCHW6ec7JMi0=
 github.com/google/gofuzz v1.2.0/go.mod h1:dBl0BpW6vV/+mYPU4Po3pmUjxk6FQPldtuIdl/M65Eg=
 github.com/google/pprof v0.0.0-20230817174616-7a8ec2ada47b h1:h9U78+dx9a4BKdQkBBos92HalKpaGKHrp+3Uo6yTodo=
+github.com/google/pprof v0.0.0-20230817174616-7a8ec2ada47b/go.mod h1:czg5+yv1E0ZGTi6S6vVK1mke0fV+FaUhNGcd6VRS9Ik=
 github.com/google/uuid v1.6.0 h1:NIvaJDMOsjHA8n1jAhLSgzrAzy1Hgr+hNrb57e+94F0=
 github.com/google/uuid v1.6.0/go.mod h1:TIyPZe4MgqvfeYDBFedMoGGpEw/LqOeaOT+nhxU+yHo=
 github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 h1:+9834+KizmvFV7pXQGSXQTsaWhq2GjuNUt0aUU0YBYw=
+github.com/grpc-ecosystem/go-grpc-middleware v1.3.0/go.mod h1:z0ButlSOZa5vEBq9m2m2hlwIgKw+rp3sdCBRoJY+30Y=
 github.com/hashicorp/errwrap v1.0.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
 github.com/hashicorp/errwrap v1.1.0 h1:OxrOeh75EUXMY8TBjag2fzXGZ40LB6IKw45YeGUDY2I=
 github.com/hashicorp/errwrap v1.1.0/go.mod h1:YH+1FKiLXxHSkmPseP+kNlulaMuP3n2brvKWEqk/Jc4=
@@ -142,6 +151,7 @@ github.com/hashicorp/go-multierror v1.0.0/go.mod h1:dHtQlpGsu+cZNNAkkCN/P3hoUDHh
 github.com/hashicorp/go-multierror v1.1.1 h1:H5DkEtf6CXdFp0N0Em5UCwQpXMWke8IA0+lD48awMYo=
 github.com/hashicorp/go-multierror v1.1.1/go.mod h1:iw975J/qwKPdAO1clOe2L8331t/9/fmwbPZ6JB6eMoM=
 github.com/hexops/gotextdiff v1.0.3 h1:gitA9+qJrrTCsiCl7+kh75nPqQt1cx4ZkudSTLoUqJM=
+github.com/hexops/gotextdiff v1.0.3/go.mod h1:pSWU5MAI3yDq+fZBTazCSJysOMbxWL1BSow5/V2vxeg=
 github.com/jmespath/go-jmespath v0.4.0 h1:BEgLn5cpjn8UN1mAw4NjwDrS35OdebyEtFe+9YPoQUg=
 github.com/jmespath/go-jmespath v0.4.0/go.mod h1:T8mJZnbsbmF+m6zOOFylbeCJqk5+pHWvzYPziyZiYoo=
 github.com/jmespath/go-jmespath/internal/testify v1.5.1 h1:shLQSRRSCCPj3f2gpwzGwWFoC7ycTf1rcQZHOlsJ6N8=
@@ -151,7 +161,9 @@ github.com/johannesboyne/gofakes3 v0.0.0-20230506070712-04da935ef877/go.mod h1:A
 github.com/klauspost/compress v1.17.5 h1:d4vBd+7CHydUqpFBgUEKkSdtSugf9YFmSkvUYPquI5E=
 github.com/klauspost/compress v1.17.5/go.mod h1:/dCuZOvVtNoHsyb+cuJD3itjs3NbnF6KH9zAO4BDxPM=
 github.com/kr/pretty v0.2.1 h1:Fmg33tUaq4/8ym9TJN1x7sLJnHVwhP33CNkpYV/7rwI=
+github.com/kr/pretty v0.2.1/go.mod h1:ipq/a2n7PKx3OHsz4KJII5eveXtPO4qwEXGdVfWzfnI=
 github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY=
+github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE=
 github.com/krolaw/zipstream v0.0.0-20180621105154-0a2661891f94 h1:+AIlO01SKT9sfWU5CLWi0cfHc7dQwgGz3FhFRzXLoMg=
 github.com/krolaw/zipstream v0.0.0-20180621105154-0a2661891f94/go.mod h1:TcE3PIIkVWbP/HjhRAafgCjRKvDOi086iqp9VkNX/ng=
 github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826 h1:RWengNIwukTxcDr9M+97sNutRR1RKhG96O6jWumTTnw=
@@ -159,6 +171,7 @@ github.com/mohae/deepcopy v0.0.0-20170929034955-c48cc78d4826/go.mod h1:TaXosZuwd
 github.com/oklog/ulid/v2 v2.1.0 h1:+9lhoxAP56we25tyYETBBY1YLA2SaoLvUFgrP2miPJU=
 github.com/oklog/ulid/v2 v2.1.0/go.mod h1:rcEKHmBBKfef9DhnvX7y1HZBYxjXb0cP5ExxNsTT1QQ=
 github.com/opentracing/opentracing-go v1.2.0 h1:uEJPy/1a5RIPAJ0Ov+OIO8OxWu77jEv+1B0VhjKrZUs=
+github.com/opentracing/opentracing-go v1.2.0/go.mod h1:GxEUsuufX4nBwe+T+Wl9TAgYrxe9dPLANfrWvHYVTgc=
 github.com/outcaste-io/ristretto v0.2.3 h1:AK4zt/fJ76kjlYObOeNwh4T3asEuaCmp26pOvUOL9w0=
 github.com/outcaste-io/ristretto v0.2.3/go.mod h1:W8HywhmtlopSB1jeMg3JtdIhf+DYkLAr0VN/s4+MHac=
 github.com/pborman/getopt v0.0.0-20170112200414-7148bc3a4c30/go.mod h1:85jBQOZwpVEaDAr341tbn15RS4fCAsIst0qp7i8ex1o=
@@ -172,6 +185,7 @@ github.com/pmezard/go-difflib v1.0.1-0.20181226105442-5d4384ee4fb2/go.mod h1:iKH
 github.com/posener/complete v1.2.3 h1:NP0eAhjcjImqslEwo/1hq7gpajME0fTLTezBKDqfXqo=
 github.com/posener/complete v1.2.3/go.mod h1:WZIdtGGp+qx0sLrYKtIRAruyNpv6hFCicSgv7Sy7s/s=
 github.com/richardartoul/molecule v1.0.1-0.20221107223329-32cfee06a052 h1:Qp27Idfgi6ACvFQat5+VJvlYToylpM/hcyLBI3WaKPA=
+github.com/richardartoul/molecule v1.0.1-0.20221107223329-32cfee06a052/go.mod h1:uvX/8buq8uVeiZiFht+0lqSLBHF+uGV8BrTv8W/SIwk=
 github.com/richardlehane/mscfb v1.0.4 h1:WULscsljNPConisD5hR0+OyZjwK46Pfyr6mPu5ZawpM=
 github.com/richardlehane/mscfb v1.0.4/go.mod h1:YzVpcZg9czvAuhk9T+a3avCpcFPMUWm7gK3DypaEsUk=
 github.com/richardlehane/msoleps v1.0.1/go.mod h1:BWev5JBpU9Ko2WAgmZEuiz4/u3ZYTKbjLycmwiWUfWg=
@@ -190,11 +204,13 @@ github.com/sirupsen/logrus v1.7.0/go.mod h1:yWOB1SBYBC5VeMP7gHvWumXLIWorT60ONWic
 github.com/sony/gobreaker v0.5.0 h1:dRCvqm0P490vZPmy7ppEk2qCnCieBooFJ+YoXGYB+yg=
 github.com/sony/gobreaker v0.5.0/go.mod h1:ZKptC7FHNvhBz7dN2LGjPVBz2sZJmc0/PkyDJOjmxWY=
 github.com/spaolacci/murmur3 v1.1.0 h1:7c1g84S4BPRrfL5Xrdp6fOJ206sU9y293DDHaoy0bLI=
+github.com/spaolacci/murmur3 v1.1.0/go.mod h1:JwIasOWyU6f++ZhiEuf87xNszmSA2myDM2Kzu9HwQUA=
 github.com/spf13/afero v1.2.1/go.mod h1:9ZxEEn6pIJ8Rxe320qSDBk6AsU0r9pR7Q4OcevTdifk=
 github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME=
 github.com/stretchr/objx v0.4.0/go.mod h1:YvHI0jy2hoMjB+UWwv71VJQ9isScKT/TqJzVSSt89Yw=
 github.com/stretchr/objx v0.5.0/go.mod h1:Yh+to48EsGEfYuaHDzXPcE3xhTkx73EhmCGUpEOglKo=
 github.com/stretchr/objx v0.5.2 h1:xuMeJ0Sdp5ZMRXx/aWO6RZxdr3beISkG5/G/aIRr3pY=
+github.com/stretchr/objx v0.5.2/go.mod h1:FRsXN1f5AsAjCGJKqEizvkpNtU+EGNCLh3NxZ/8L+MA=
 github.com/stretchr/testify v1.2.2/go.mod h1:a8OnRcib4nhh0OaRAV+Yts87kKdq0PP7pXfy6kDkUVs=
 github.com/stretchr/testify v1.3.0/go.mod h1:M5WIy9Dh21IEIfnGCwXGc5bZfKNJtfHm1UVUgZn+9EI=
 github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4=
@@ -226,6 +242,7 @@ github.com/yuin/goldmark v1.3.5/go.mod h1:mwnBkeHKe2W/ZEtQ+71ViKU8L12m81fl3OWwC1
 github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY=
 go.etcd.io/bbolt v1.3.5/go.mod h1:G5EMThwa9y8QZGBClrRx5EY+Yw9kAhnjy3bSjsnlVTQ=
 go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.44.0 h1:KfYpVmrjI7JuToy5k8XV3nkapjWx48k4E4JOtVstzQI=
+go.opentelemetry.io/contrib/instrumentation/net/http/otelhttp v0.44.0/go.mod h1:SeQhzAEccGVZVEy7aH87Nh0km+utSpo1pTv6eMMop48=
 go.opentelemetry.io/otel v1.24.0 h1:0LAOdjNmQeSTzGBzduGe/rU4tZhMwL5rWgtp9Ku5Jfo=
 go.opentelemetry.io/otel v1.24.0/go.mod h1:W7b9Ozg4nkF5tWI5zsXkaKKDjdVjpD4oAt9Qi/MArHo=
 go.opentelemetry.io/otel/metric v1.24.0 h1:6EhoGWWK28x1fbpA4tYTOWBkPefTDQnb8WSGXlc88kI=
@@ -236,6 +253,7 @@ go.uber.org/atomic v1.9.0/go.mod h1:fEN4uk6kAWBTFdckzkM89CLk9XfWZrxpCo0nPH17wJc=
 go.uber.org/atomic v1.11.0 h1:ZvwS0R+56ePWxUNi+Atn9dWONBPp/AUETXlHW0DxSjE=
 go.uber.org/atomic v1.11.0/go.mod h1:LUxbIzbOniOlMKjJjyPfpl4v+PKK2cNJn91OQbhoJI0=
 go.uber.org/goleak v1.3.0 h1:2K3zAYmnTNqV73imy9J1T3WC+gmCePx2hEGkimedGto=
+go.uber.org/goleak v1.3.0/go.mod h1:CoHD4mav9JJNrW/WLlf7HGZPjdw8EucARQHekz1X6bE=
 golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w=
 golang.org/x/crypto v0.0.0-20191011191535-87dc89f01550/go.mod h1:yigFU9vqHzYiE8UmvKecakEJjdnWj3jj499lnFckfCI=
 golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc=
@@ -265,6 +283,7 @@ golang.org/x/sync v0.0.0-20210220032951-036812b2e83c/go.mod h1:RxMgew5VJxzue5/jJ
 golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sync v0.6.0 h1:5BMeUDZ7vkXGfEr1x9B4bRcTH4lpkTkpdh0T/J+qjbQ=
+golang.org/x/sync v0.6.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190412213103-97732733099d/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
 golang.org/x/sys v0.0.0-20191026070338-33540a1f6037/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
@@ -325,11 +344,14 @@ gopkg.in/DataDog/dd-trace-go.v1 v1.63.1 h1:POnTNQLAJHnuywfk48N+l/EiwQJ6Kdaa7nwV5
 gopkg.in/DataDog/dd-trace-go.v1 v1.63.1/go.mod h1:pv2V0h4+skvObjdi3pWV4k6JHsdQk+flbjdC25mmTfU=
 gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
 gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c h1:Hei/4ADfdWqJk1ZMxUNpqntNwaWcugrBjAiHlqqRiVk=
+gopkg.in/check.v1 v1.0.0-20201130134442-10cb98267c6c/go.mod h1:JHkPIbrfpd72SG/EVd6muEfDQjcINNoR0C8j2r3qZ4Q=
 gopkg.in/mgo.v2 v2.0.0-20180705113604-9856a29383ce/go.mod h1:yeKp02qBN3iKW1OzL3MGk2IdtZzaj7SFntXj72NppTA=
 gopkg.in/yaml.v2 v2.2.2/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.2.8/go.mod h1:hI93XBmqTisBFMUTm0b8Fm+jr3Dg1NNxqwp+5A1VGuI=
 gopkg.in/yaml.v2 v2.4.0 h1:D8xgwECY7CYvx+Y2n4sBz93Jn9JRvxdiyyo8CTfuKaY=
+gopkg.in/yaml.v2 v2.4.0/go.mod h1:RDklbk79AGWmwhnvt/jBztapEOGDOx6ZbXqjP6csGnQ=
 gopkg.in/yaml.v3 v3.0.0-20200313102051-9f266ea9e77c/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
 gopkg.in/yaml.v3 v3.0.1/go.mod h1:K4uyk7z7BCEPqu6E+C64Yfv1cQ7kz7rIZviUmN+EgEM=
 honnef.co/go/gotraceui v0.2.0 h1:dmNsfQ9Vl3GwbiVD7Z8d/osC6WtGGrasyrC2suc4ZIQ=
+honnef.co/go/gotraceui v0.2.0/go.mod h1:qHo4/W75cA3bX0QQoSvDjbJa4R8mAyyFjbWAj63XElc=

I can also dig into the terraform hcl diff, but kind of want to take them one at a time, in case one affects the next.

@TylerHendrickson
Copy link
Member

@TylerHendrickson On a clean main branch, when I run go mod tidy I do end up with changes to go.mod and go.sum. They appear to be related to the golang version (e.g., the diff switches go 1.20 to go 1.21 in go.mod). But confusingly, I'm seeing different versions in different places.

  • At the terminal, go version shows go version go1.22.1 darwin/arm64
  • When I run go mod tidy, the diff to go.mod shows go 1.20 changed to go 1.21 but adds toolchain go1.22.1

Why am I seeing 1.21 and 1.22? And I guess more to the point, what are the standard tools for managing multiple versions of go for different projects? How do I best make sure I'm using 1.20 while working on this codebase?

For reference, the full diff after running go mod tidy:
I can also dig into the terraform hcl diff, but kind of want to take them one at a time, in case one affects the next.

@jeffsmohan My guess is that resolving this

Why am I seeing 1.21 and 1.22? And I guess more to the point, what are the standard tools for managing multiple versions of go for different projects? How do I best make sure I'm using 1.20 while working on this codebase?

will ensure that go mod tidy doesn't make changes on a clean branch. I use ephemeral dev environments for this sort of thing, so I only ever have one version of Go (1.20) installed in my environment by default. If you're running bare go commands, then it's just going to use whatever go is in your $PATH; I know of a few strategies for having multiple versions of Go installed on a system:

@jeffsmohan
Copy link
Contributor Author

@TylerHendrickson Ensuring I'm running go v1.20 avoids the issues with go mod tidy updating those files. 🎉

Do you think it's worth doing anything to prevent future travelers running into this? (Not sure if there's an equivalent of .nvmrc file? Or just something worth documenting in the README?) Or for someone sufficiently familiar with go, is this line in go.mod enough to key them in that they should be using v1.20? Genuinely not sure, as I'm brand new to the world of go.

Now on to terraform... when I run tflocal init -backend-config="local.s3.tfbackend" -reconfigure (per the README, on a fresh checkout of main branch), this command updates the terraform/.terraform.lock.hcl file. Any idea why?

Here's the full diff:
diff --git a/terraform/.terraform.lock.hcl b/terraform/.terraform.lock.hcl
index e7e923a..46c0d0d 100644
--- a/terraform/.terraform.lock.hcl
+++ b/terraform/.terraform.lock.hcl
@@ -6,6 +6,7 @@ provider "registry.terraform.io/datadog/datadog" {
   constraints = "~> 3.35.0"
   hashes = [
     "h1:dh/i0YaYXH/GMYOdaq3l24ugn7BKRPLz/y4bQjhjqSQ=",
+    "h1:u6FnxwYiiTDvZ56DSBK3xL9xQvpdZM/679utP1yat2A=",
     "h1:wfDwvIjOdXDMKX8SUxQrq9utgg8OjOKypRVFB9cIJiI=",
     "zh:00af9b501ee346ffcbe415810a29f22ecc4e88226c7d51e56ec45a12fe3cf62c",
     "zh:2af7676af64513b0153c1023ebdd51541ddf4b6669e44fef1c2534ad80a7bb39",
@@ -28,6 +29,7 @@ provider "registry.terraform.io/hashicorp/archive" {
   version     = "2.4.1"
   constraints = "2.4.1"
   hashes = [
+    "h1:3mCpFxc6HwDIETCFHNENlxBUgKdsW2S1EmVHARn9Lgk=",
     "h1:AABk7Bon/bM1LKziIsGG78d97c9IhEqh/EWJs1J0rLo=",
     "h1:JgIo+nNySG8svjXevfoTRi0jzgHbLMDrnr55WBeRupw=",
     "zh:00240c042740d18d6ba545b211ff7ed5a9e8490d30be3f865e71dba90d7a34cf",
@@ -50,6 +52,7 @@ provider "registry.terraform.io/hashicorp/aws" {
   constraints = ">= 2.0.0, >= 4.0.0, >= 4.9.0, >= 4.59.0, >= 4.63.0, ~> 5.46.0"
   hashes = [
     "h1:GK1y1qAGcPHPZxz01Ko6v+815T7kZPXt6POBsLg9c/k=",
+    "h1:d0Mf33mbbQujZ/JaYkqmH5gZGvP+iEIWf9yBSiOwimE=",
     "h1:gagAtniijwJRhsKRBWWZfmnPiqu4u1A5oI626+KA/1g=",
     "zh:05ae6180a7f23071435f6e5e59c19af0b6c5da42ee600c6c1568c8660214d548",
     "zh:0d878d1565d5e57ce6b34ec5f04b28662044a50c999ec5770c374aa1f1020de2",
@@ -75,6 +78,7 @@ provider "registry.terraform.io/hashicorp/external" {
   hashes = [
     "h1:H+3QlVPs/7CDa3I4KU/a23wYeGeJxeBlgvR7bfK1t1w=",
     "h1:Qi72kOSrEYgEt5itloFhDfmiFZ7wnRy3+F74XsRuUOw=",
+    "h1:gShzO1rJtADK9tDZMvMgjciVAzsBh39LNjtThCwX1Hg=",
     "zh:03d81462f9578ec91ce8e26f887e34151eda0e100f57e9772dbea86363588239",
     "zh:37ec2a20f6a3ec3a0fd95d3f3de26da6cb9534b30488bc45723e118a0911c0d8",
     "zh:4eb5b119179539f2749ce9de0e1b9629d025990f062f4f4dddc161562bb89d37",
@@ -96,6 +100,7 @@ provider "registry.terraform.io/hashicorp/http" {
   hashes = [
     "h1:eqo0hkFNrixeaT93PC5NiU893s7rUwwOMeqnCjjj3u0=",
     "h1:v6Hn+15SfN2SI281Sp+uNXdWhD197ycP07fnaoGpPcc=",
+    "h1:vaoPfsLm6mOk6avKTrWi35o+9p4fEeZAY3hzYoXVTfo=",
     "zh:0ba051c9c8659ce0fec94a3d50926745f11759509c4d6de0ad5f5eb289f0edd9",
     "zh:23e6760e8406fef645913bf47bfab1ca984c1c5805d2bb0ef8310b16913d29cd",
     "zh:3c69fde4548bfe65b968534c4df8d699648c921d6a065b97fec5faece73a442b",
@@ -117,6 +122,7 @@ provider "registry.terraform.io/hashicorp/local" {
   hashes = [
     "h1:Bs7LAkV/iQTLv72j+cTMrvx2U3KyXrcVHaGbdns1NcE=",
     "h1:R97FTYETo88sT2VHfMgkPU3lzCsZLunPftjSI5vfKe8=",
+    "h1:ZUEYUmm2t4vxwzxy1BvN1wL6SDWrDxfH7pxtzX8c6d0=",
     "zh:53604cd29cb92538668fe09565c739358dc53ca56f9f11312b9d7de81e48fab9",
     "zh:66a46e9c508716a1c98efbf793092f03d50049fa4a83cd6b2251e9a06aca2acf",
     "zh:70a6f6a852dd83768d0778ce9817d81d4b3f073fab8fa570bff92dcb0824f732",
@@ -138,6 +144,7 @@ provider "registry.terraform.io/hashicorp/null" {
   hashes = [
     "h1:FbGfc+muBsC17Ohy5g806iuI1hQc4SIexpYCrQHQd8w=",
     "h1:tSj1mL6OQ8ILGqR2mDu7OYYYWf+hoir0pf9KAQ8IzO8=",
+    "h1:ydA0/SNRVB1o95btfshvYsmxA+jZFRZcvKzZSB+4S1M=",
     "zh:58ed64389620cc7b82f01332e27723856422820cfd302e304b5f6c3436fb9840",
     "zh:62a5cc82c3b2ddef7ef3a6f2fedb7b9b3deff4ab7b414938b08e51d6e8be87cb",
     "zh:63cff4de03af983175a7e37e52d4bd89d990be256b16b5c7f919aff5ad485aa5",
@@ -159,6 +166,7 @@ provider "registry.terraform.io/hashicorp/time" {
   hashes = [
     "h1:NUv/YtEytDQncBQ2mTxnUZEy/rmDlPYmE9h2iokR0vk=",
     "h1:UHcDnIYFZ00uoou0TwPGMwOrE8gTkoRephIvdwDAK70=",
+    "h1:VxyoYYOCaJGDmLz4TruZQTSfQhvwEcMxvcKclWdnpbs=",
     "zh:00a1476ecf18c735cc08e27bfa835c33f8ac8fa6fa746b01cd3bcbad8ca84f7f",
     "zh:3007f8fc4a4f8614c43e8ef1d4b0c773a5de1dcac50e701d8abc9fdc8fcb6bf5",
     "zh:5f79d0730fdec8cb148b277de3f00485eff3e9cf1ff47fb715b1c969e5bbd9d4",

@jeffsmohan jeffsmohan force-pushed the jmo-ingest-forecasted branch from a833cfc to 971a340 Compare June 26, 2024 17:02
@jeffsmohan
Copy link
Contributor Author

@TylerHendrickson I also added the flag we discussed and some tests, but I'm still having trouble getting the tests working correctly. I'm hoping you can take a look at the code I just pushed up and offer some direction on where I might've gone wrong? Or I'm happy to grab some time for a 15-minute pairing session if that's easier

@TylerHendrickson
Copy link
Member

@jeffsmohan

Do you think it's worth doing anything to prevent future travelers running into this? (Not sure if there's an equivalent of .nvmrc file? Or just something worth documenting in the README?) Or for someone sufficiently familiar with go, is this line in go.mod enough to key them in that they should be using v1.20? Genuinely not sure, as I'm brand new to the world of go.

I (probably hastily) assumed that the line you linked to in go.mod would guide things in the right direction, but 1) I'm naturally biased and 2) won't claim I've thought about it all that much 🙂 . While the README does mention that we target Go 1.20.x at runtime, it's probably worth calling out again/more explicitly in the Development section.

I'm not aware of any kind of .nvmrc-like convention built into Go directly, but goenv respects a .go-version file. It might (also) be worth adding that to this repo, regardless of how strongly we suggest that folks use that specific tool during development. goversion does not seem to have that kind of convention, FYI.

Now on to terraform... when I run tflocal init -backend-config="local.s3.tfbackend" -reconfigure (per the README, on a fresh checkout of main branch), this command updates the terraform/.terraform.lock.hcl file. Any idea why?

This one has me a bit stumped, as I've not been able to reproduce this behavior. Are you using Terraform 1.5.1? I expect so, because the required_version line in terraform/main.tf ought to be enforcing that. The diff you provided appears to only be adding a new checksum for certain dependencies (as opposed to outright modifying a version like using -upgrade might do). I might need to spend some time looking into Terraform's lockfile documentation to figure out exactly what might be going on here.

@TylerHendrickson I also added the flag we discussed and some tests, but I'm still having trouble getting the tests working correctly. I'm hoping you can take a look at the code I just pushed up and offer some direction on where I might've gone wrong? Or I'm happy to grab some time for a 15-minute pairing session if that's easier

I will look at this first thing tomorrow and report back!

@jeffsmohan
Copy link
Contributor Author

jeffsmohan commented Jun 28, 2024

I (probably hastily) assumed that the line you linked to in go.mod would guide things in the right direction, but 1) I'm naturally biased and 2) won't claim I've thought about it all that much 🙂 . While the README does mention that we target Go 1.20.x at runtime, it's probably worth calling out again/more explicitly in the Development section.

Makes total sense — and we don't need to overcorrect here if we're doing the right things for someone with Go experience to get started efficiently. Maybe I'll just throw together a quick PR adding a mention to the Development prerequisites section. (edit: PR here #864)

Are you using Terraform 1.5.1?

Yep, I am. Fwiw, this is not blocking — I can just make sure I don't check in the changes to that file myself — but wanted to see if it was something easily addressed. (I'm curious, if you apply my diff to your lock file, then run tflocal init on your setup, does it revert the changes?)

Copy link
Member

@TylerHendrickson TylerHendrickson left a comment

Choose a reason for hiding this comment

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

@jeffsmohan Two test-related fixes, but aside from that I think this is looking good!

Comment on lines 322 to 331
// Configure forecasted flag in environment variables
if tt.isForecastedGrantsEnabled {
goenv.Unmarshal(goenv.EnvSet{
"IS_FORECASTED_GRANTS_ENABLED": "true",
}, &env)
} else {
goenv.Unmarshal(goenv.EnvSet{
"IS_FORECASTED_GRANTS_ENABLED": "false",
}, &env)
}
Copy link
Member

Choose a reason for hiding this comment

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

These goenv.Unmarshal calls are responsible for some of the current test failures. In short, they reconfigure the referenced &env interface using the provided EnvSet, which is missing certain environment variables.

To break it down:

  1. The Environment struct definition for this package is annotated to require a GRANTS_PREPARED_DATA_BUCKET_NAME env var:
    DestinationBucket string `env:"GRANTS_PREPARED_DATA_BUCKET_NAME,required=true"`
    but that's missing from the mock environment (goenv.EnvSet) defined here, so the calls to goenv.Unmarshal() are failing – we're just not seeing the failure because the error value that goenv.Unmarshal() returns is being ignored here. However, if we were to do something like this:
    	    if tt.isForecastedGrantsEnabled {
    		    err := goenv.Unmarshal(goenv.EnvSet{
    			    "IS_FORECASTED_GRANTS_ENABLED": "true",
    		    }, &env)
    		    require.NoError(tt, err)
    	    } else {
    		    err := goenv.Unmarshal(goenv.EnvSet{
    			    "IS_FORECASTED_GRANTS_ENABLED": "false",
    		    }, &env)
    		    require.NoError(tt, err)
    	    }
    we'd see that err is actually not nil and our require.NoError() call would make the test fail:
     Error:    Received unexpected error:
        	   value for this field is required [GRANTS_PREPARED_DATA_BUCKET_NAME]
    
    Note: For reference, this error is of type goenv.ErrMissingRequiredValue.
  2. Our test setup logic uses env.DestinationBucket when it's creating the mock S3 buckets we use during test execution:
    client := s3.NewFromConfig(cfg, func(o *s3.Options) { o.UsePathStyle = true })
    ctx := context.TODO()
    bucketsToCreate := []string{sourceBucketName, env.DestinationBucket}
    for _, bucketName := range bucketsToCreate {
    _, err := client.CreateBucket(ctx, &s3.CreateBucketInput{Bucket: aws.String(bucketName)})
    if err != nil {
    return client, cfg, err
    }
    }
    return client, cfg, nil
    which is why we're seeing S3 API calls failing later in the tests.

It's useful to note that our test helper function, setupLambdaEnvForTesting(), is taking care of populating the global env variable with sensible default values for testing. So rather than calling goenv.Unmarshal() again, we can just set env.IsForecastedGrantsEnabled directly:

Suggested change
// Configure forecasted flag in environment variables
if tt.isForecastedGrantsEnabled {
goenv.Unmarshal(goenv.EnvSet{
"IS_FORECASTED_GRANTS_ENABLED": "true",
}, &env)
} else {
goenv.Unmarshal(goenv.EnvSet{
"IS_FORECASTED_GRANTS_ENABLED": "false",
}, &env)
}
env.IsForecastedGrantsEnabled = tt.isForecastedGrantsEnabled

Comment on lines 214 to 221
{
SOURCE_FORECAST_TEMPLATE,
"2345",
now.AddDate(-1, 0, 0).Format("01022006"),
false,
false,
true,
},
Copy link
Member

Choose a reason for hiding this comment

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

This isValid value needs to be switched to true given that this grantValues array member is meant to generate well-formed source XML. After resolving the env var-related issue, this test case is failing An error is expected but got nil. due to the require.Error() assertion in this snippet:

sourceContainsInvalidOpportunities := false
for _, v := range tt.grantValues {
if !v.isValid {
sourceContainsInvalidOpportunities = true
}
}
if sourceContainsInvalidOpportunities {
require.Error(t, invocationErr)
} else {
require.NoError(t, invocationErr)
}

Suggested change
{
SOURCE_FORECAST_TEMPLATE,
"2345",
now.AddDate(-1, 0, 0).Format("01022006"),
false,
false,
true,
},
{
SOURCE_FORECAST_TEMPLATE,
"2345",
now.AddDate(-1, 0, 0).Format("01022006"),
false,
true,
true,
},

@TylerHendrickson
Copy link
Member

@jeffsmohan

(I'm curious, if you apply my diff to your lock file, then run tflocal init on your setup, does it revert the changes?)

Nope, with your diff applied, tflocal init does not modify the lock file. I tested with a few different permutations and got the same result (file unmodified) each time:

  • With and without the -reconfigure flag
  • An this branch as well as main
  • With tflocal init as well as terraform init (using -backend=false for the latter)

@jeffsmohan
Copy link
Contributor Author

jeffsmohan commented Jul 2, 2024

@jeffsmohan Two test-related fixes, but aside from that I think this is looking good!

Thanks, that really helped unstick me! Note that even though your fixes got tests passing, there was still an issue wherein the "When flag is disabled, ignores well-formed source XML for single new forecast" test for when the enabled flag is false was not actually testing what we wanted. This came down to a couple issues:

  1. The test was actually verifying the S3 file was saved, rather than testing that it was skipped, as the test name implies and as we actually want when testing the enabled flag.
  2. Meanwhile, the test was still passing because I was reusing opportunity IDs between test cases. (I didn't realize there would be pollution in S3 between the test cases.) So a previous test case was actually saving a file in S3 in the expected location.

In the end, this "double negative" ended up with a passing test even though it wasn't testing what we wanted. I fixed the above two issues, as well as adding a check in the test setup to fail the test with a helpful message if it spots a duplicated opportunity ID. (Though do lmk if that's not the best approach!)

And with that, @TylerHendrickson , I thiiiiiink this one is finally ready to land! Thanks for your patience!

Copy link
Member

@TylerHendrickson TylerHendrickson left a comment

Choose a reason for hiding this comment

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

Looks great – approved! 🎉

@jeffsmohan jeffsmohan enabled auto-merge (squash) July 10, 2024 17:58
@jeffsmohan jeffsmohan merged commit ef138fc into main Jul 10, 2024
19 checks passed
@jeffsmohan jeffsmohan deleted the jmo-ingest-forecasted branch July 10, 2024 18:03
@TylerHendrickson TylerHendrickson added the enhancement New feature or request label Sep 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request go Pull requests that update Go code terraform Pull requests that update Terraform code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants