Modernized terraform script for deployment as a module of nasa-pds/registry#765
Conversation
| } | ||
|
|
||
| # Add a Pull Through Cache rule for GHCR | ||
| resource "aws_ecr_pull_through_cache_rule" "ghcr" { |
There was a problem hiding this comment.
Wow this is really cool. I had no idea this resource even existed.
There was a problem hiding this comment.
Unfortunatly I was unable to make it work with a terrafrom deployment, I'll have to come back to it later.
nutjob4life
left a comment
nutjob4life
left a comment
There was a problem hiding this comment.
Excellent modernization of the Terraform structure that should make it more maintanable and easier to use. Just a few nits (see inline comments). Thanks!
| .terraform/ | ||
| terraform/*.tfvars | ||
| !terraform/*.tfvars.example | ||
| terraform/.terraform.lock.hcl |
There was a problem hiding this comment.
I believe the .terraform.lock.hcl has the same purpose as the package-lock.json in Node projects and exists to pin provider versions, avoiding the "works on my machine" issue. AI agrees and says .terraform.lock.hcl should be commited and the .gitingore line should be
!.terraform.lock.hcl
|
|
||
| # Look up the secret when it is not created by this script | ||
| data "aws_secretsmanager_secret" "github_ecr_credentials" { | ||
| count = 1 - var.create_github_secret_credentials |
There was a problem hiding this comment.
Might be safer to to do
type = bool
default = trueand do
count = var.create_github_secret_credentials ? 1 : 0on the off chance that someone sets create_github_secret_credentials = 2, for example.
| count = var.create_github_secret_credentials | ||
|
|
||
| secret_id = aws_secretsmanager_secret.github_ecr_credentials[count.index].id | ||
| secret_string = jsonencode({ |
There was a problem hiding this comment.
Might want to add a warning note to the README that the GitHub username and token become part of the Terraform state.
| @@ -0,0 +1,19 @@ | |||
| locals { | |||
|
|
|||
| module_relative_path = replace(abspath(path.module), "/^.*\\/terraform(\\/|$)/", "") | |||
There was a problem hiding this comment.
Does this work? I didn't think Terraform's replace uses /.../ delimiters for regexes.
There was a problem hiding this comment.
claude code came up with that syntax and it works.
|
@tloubrieu-jpl when you have a chance can you respond to all of @nutjob4life comments and triage the sonarcloud issues? |
|
nutjob4life
left a comment
nutjob4life
left a comment
There was a problem hiding this comment.
Thanks for the updates @tloubrieu-jpl 👍
|
Numerous things called out by sonarcloud but will merge for now. Need to move this forward. |
3 participants
🗒️ Summary
Modernize the terraform code for consistency with other repository and so to be able to call the code as a module of the umbrella repository nasa-pds/registry deployment script.
Aborted attempt to use Pull Through Cache ECR configuration instead of a manual deployment of the docker image. We will have to come back to that later since it did not work.
🤖 AI Assistance Disclosure
Estimated % of code influenced by AI: _50 %
⚙️ Test Data and/or Report
Tested with a deployment on AWS and integration tests succesfully run on it.
♻️ Related Issues
Also fixes #767 (integration test fix)
🤓 Reviewer Checklist
Reviewers: Please verify the following before approving this pull request.
Documentation and PR Content
Security & Quality
Testing & Validation
Maintenance