-
Notifications
You must be signed in to change notification settings - Fork 6
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
Support new GitHub bot deployment #15
Conversation
ChengaDev
commented
May 1, 2024
•
edited
Loading
edited
- Enable deployment of our new GitHub bot via Terraform
- Enable several instances of the bot in a single region
- Enable set custom name pattern for all module resources
- Allow the user to set a path to the required source code files he wants to deploy to the lambdas
- Update runtime to nodejs20.x
@@ -152,3 +152,7 @@ module "spectral_lambda_integration" { | |||
8. `lambda_iam_role_arn` - Amazon Resource Name (ARN) specifying the role. | |||
9. `lambda_iam_role_name` - Name of the role. | |||
10. `secrets_arns` - Arns of created secrets in secrets manager. | |||
|
|||
## Support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding a changelog file that lists versions, describes the changes, and breaks changes per version is essential, and can avoid a lot of questions.
@@ -14,14 +14,14 @@ Terraform configuration used to create the required AWS resources for integratin | |||
|
|||
| Name | Version | | |||
| ----------- | ----------- | | |||
| [aws](https://registry.terraform.io/providers/hashicorp/aws/latest/docs) | >= 4.37.0 | | |||
| [aws](https://registry.terraform.io/providers/hashicorp/aws/latest/docs) | >= 5.26.0 | | |||
|
|||
## Inputs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is some missing fields in the documentation. check out this tool https://terraform-docs.io/ it can help you to manage it
lambda_source_code_path
frontend_lambda_source_code_path
backend_lambda_source_code_path
lambda_logs_retention_in_days
lambda_enable_logs
resource_name_common_part
secrets_names
gateway_api_integration_timeout_milliseconds
@@ -30,7 +30,7 @@ Terraform configuration used to create the required AWS resources for integratin | |||
| `lambda_function_timeout` | Amount of time your Lambda Function has to run in seconds. | `number` | 300 | No | | |||
| `lambda_function_memory_size` | Amount of memory in MB your Lambda Function can use at runtime. | `number` | 1024 | No | | |||
| `lambda_publish` | Whether to publish creation/change as new Lambda Function Version. | `bool` | `false` | No | | |||
| `store_secret_in_secrets_manager` | Whether to store secrets values on a vault (currently supporting AWS secrets manager on GitLab integration). | `bool` | `false` | No | | |||
| `store_secret_in_secrets_manager` | Whether to store secrets values on a vault (currently supporting AWS secrets manager on GitHub / GitLab integration). | `bool` | `false` | No | | |||
|
|||
### env_vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section is partially relevant to the bots.
i think we can remove this section and redirect to our guide docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The env vars are not detailed in this section, this just shows how to set variables and redirects the users to the docs.
The only env var mentioned here is SPECTRAL_DSN which is always mandatory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is the var that is mandatory, in a different but, there is more mandatory env var. i just say to refer to our docs and remote this section
api_triggered_function_arn = local.single_lambda_integration ? module.lambda_function[0].lambda_function_arn : module.frontend_lambda_function[0].lambda_function_arn | ||
frontend_lambda_handler = contains(["github"], var.integration_type) ? "index.handler" : "frontend.app" | ||
backend_lambda_handler = contains(["github"], var.integration_type) ? "index.handler" : "backend.app" | ||
default_secrets_names = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what about SPECTRAL_DSN
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct
locals.tf
Outdated
"gitlab" = coalesce(var.secrets_names, ["Spectral_GitlabBot_GitlabToken", "Spectral_GitlabBot_WebhookSecret"]) | ||
} | ||
# Please do not change or replace the 'frontend' suffix since there a logic in the bot based in it | ||
function_name = local.single_lambda_integration ? local.resource_name_pattern : "${local.resource_name_pattern}-frontend" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
function_name
can be confusing since it implies working with both the backend and the front end. I recommend the following changes:
Rename options:
- receive_hook_lambda_name
- provider_request_treigger_lambda_name
The idea is to understand when you read this variable what he does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed to api_integrated_function_name
variable "lambda_source_code_filename" { | ||
type = string | ||
description = "The lambda source code filename" | ||
} | ||
|
||
variable "lambda_source_code_path" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use terraform-docs to generate the readme file for the lambda module.
this is relevant for all the modules
multiple-lambdas-integration.tf
Outdated
environment = var.environment | ||
integration_type = var.integration_type | ||
# Please do not change or replace the 'frontend' suffix since there a logic in the bot based in it | ||
resource_name_pattern = "${local.resource_name_pattern}-frontend" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently have ${local.resource_name_pattern}-frontend in the locals.
Can we use the same variable to reduce the string concatenations?
shared.tf
Outdated
} | ||
|
||
module "lambda_role" { | ||
source = "./modules/role" | ||
resource_name_pattern = local.single_lambda_integration ? local.resource_name_pattern : "${local.resource_name_pattern}-frontend" | ||
role_name = local.function_name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is the role name getting the function name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this role and its policies were created explicitly for this lambda function, and with this function name there will be no doubt about why this role was created and who should use it