-
Notifications
You must be signed in to change notification settings - Fork 16
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
feat: Added SQS support to ArmoniK #1344
base: main
Are you sure you want to change the base?
Conversation
13ffd16
to
7b1f85c
Compare
7b1f85c
to
6879363
Compare
I would need to upgrade the version of Core to 0.29.1, and the ArmoniK.Infra PR would need to be merged first before merging this |
8929679
to
47ecef9
Compare
@@ -143,6 +144,24 @@ module "mq" { | |||
kms_key_id = local.kms_key | |||
} | |||
|
|||
module "sqs" { | |||
count = length(var.sqs_service_account_name) > 0 ? 1 : 0 |
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.
count = length(var.sqs_service_account_name) > 0 ? 1 : 0 | |
count = can(coalesce(var.sqs_service_account_name)) ? 1 : 0 |
} | ||
|
||
module "sqs_service_account" { | ||
count = length(var.sqs_service_account_name) > 0 ? 1 : 0 |
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.
count = length(var.sqs_service_account_name) > 0 ? 1 : 0 | |
count = can(colaesce(var.sqs_service_account_name)) ? 1 : 0 |
@@ -76,4 +78,6 @@ module "armonik" { | |||
image = local.ecr_images["${var.pod_deletion_cost.image}:${try(coalesce(var.pod_deletion_cost.tag), "")}"].image | |||
tag = local.ecr_images["${var.pod_deletion_cost.image}:${try(coalesce(var.pod_deletion_cost.tag), "")}"].tag | |||
}) | |||
|
|||
depends_on = [module.sqs_service_account] |
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.
Since this module is optional, how is this dependency managed when the module is not deployed ?
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.
Wouldn't it be okay because the module is still deployed (just with count 0).. It's the same thing as the dependency you did with both mongodb and mongodb_sharded no? (We pass the two in even though we only deploy one of them)
versions.tfvars.json
Outdated
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.
I don't think such a PR should change so much versions
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're right.. I think I should wait for the latest version of Core to be used in ArmoniK before merging though (and also of ArmoniK.Infra) since both are needed.
type = string | ||
default = "" |
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.
type = string | |
default = "" | |
type = optional(string) |
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.
Keyword "optional" can only be used for object type attributes..
1edc0b4
to
2784f1a
Compare
2784f1a
to
686c633
Compare
686c633
to
7c33da5
Compare
Quality Gate passedIssues Measures |
Motivation
SQS was added to ArmoniK.Core but was never supported in ArmoniK.Infra, now that SQS is added as a module to ArmoniK.Infra. This PR makes adjustments to ArmoniK to support these upcoming changes;
Description
Created instances of the SQS module/service account and made it toggleable through a variable. Since integrating SQS required creating a service account that will be used by ArmoniK's control and compute planes.
Testing
Tested, queue is being created, used and tasks are executed in ArmoniK.
Impact
TODO.
Additional Information
Not Applicable.
Checklist