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

[CES-214] Added new storage account iopweumessagesnotifst for ITN migration #1265

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

cfcuffari120400
Copy link
Contributor

@cfcuffari120400 cfcuffari120400 commented Oct 29, 2024

Motivation and Context

Storage accounts must be replicated to italy north in view of the infrastructure migration

Major Changes

Create replication of iopweumessagesnotifst in italy north

Dependencies

Testing

Documentation

Other Considerations

Comment on lines 21 to 22
prefix = "io"
env_short = "p"
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefix and env_short can be gotten from the same variables with var.prefix and var.env_short

Suggested change
prefix = "io"
env_short = "p"

force_public_network_access_enabled = true

access_tier = "Hot"

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines 28 to 29
prefix = local.prefix
env_short = local.env_short
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
prefix = local.prefix
env_short = local.env_short
prefix = var.prefix
env_short = var.env_short

Comment on lines 35 to 42
tags = {
CostCenter = "TS310 - PAGAMENTI & SERVIZI"
CreatedBy = "Terraform"
Environment = "Prod"
Owner = "IO"
ManagementTeam = "Comunicazione"
Source = "https://github.com/pagopa/io-infra/blob/4b82c3e0296174eb27ddbebdb15b4cad17e19430/src/domains/messages-common/04_storage.tf#L35"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tags can be gotten from the variable var.tags

Suggested change
tags = {
CostCenter = "TS310 - PAGAMENTI & SERVIZI"
CreatedBy = "Terraform"
Environment = "Prod"
Owner = "IO"
ManagementTeam = "Comunicazione"
Source = "https://github.com/pagopa/io-infra/blob/4b82c3e0296174eb27ddbebdb15b4cad17e19430/src/domains/messages-common/04_storage.tf#L35"
}

Comment on lines 94 to 109
variable "environment" {
type = object({
prefix = string
env_short = string
location = string
domain = optional(string)
instance_number = string
})

description = "Values which are used to generate resource names and location short names. They are all mandatory except for domain, which should not be used only in the case of a resource used by multiple domains."
}

variable "resource_group_common" {
type = string
description = "Name of the common resource group"
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would consider populating these values in locals

Suggested change
variable "environment" {
type = object({
prefix = string
env_short = string
location = string
domain = optional(string)
instance_number = string
})
description = "Values which are used to generate resource names and location short names. They are all mandatory except for domain, which should not be used only in the case of a resource used by multiple domains."
}
variable "resource_group_common" {
type = string
description = "Name of the common resource group"
}

@FasanoBip FasanoBip changed the title CES-214-migrate-iopweumessagesnotifst new module code [CES-214] Added new storage account iopweumessagesnotifst for ITN migration Nov 8, 2024
Copy link

sonarqubecloud bot commented Dec 4, 2024

tier = "l"
resource_group_name = "${local.prefix}-${local.env_short}-${local.location}-messages-notifications-rg"

force_public_network_access_enabled = true
Copy link
Contributor

Choose a reason for hiding this comment

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

I know it's currently enabled, but I would disable it in the new one.
@lucacavallaro @Garma00 sounds good?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants