-
Notifications
You must be signed in to change notification settings - Fork 0
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
Terraform add #106
Terraform add #106
Conversation
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.
Couple of questions:
- What does .terraform.lock.hcl do?
- Does
main.tfplan
need to be committed? Or is committingmain.tf
sufficient? Since.tfplan
is generated from.tf
, so the.tfplan
can be generated by the build step. - What's the difference between
/test
and/init
directories?
Nice work btw!
terraform/init/variables.tf
Outdated
default = "southeastasia" | ||
description = "Location of the resource group." |
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.
Is description
a required field? If not, is it necessary to specify the description?
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.
It is not a required field. Can be removed since it's straightforward
terraform/init/variables.tf
Outdated
# Shortnames for regions can be found here: | ||
# https://github.com/claranet/terraform-azurerm-regions/blob/master/REGIONS.md | ||
variable "location" { | ||
default = "southeastasia" |
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.
Indentation looks off compared to rest of the files. Is there a way to auto-indent these files?
terraform/init/variables.tf
Outdated
variable "prefix" { | ||
default = "test" | ||
description = "Prefix of the resource group name that's combined with a random ID so name is unique in your Azure subscription." | ||
} |
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.
Can the prefix
be dynamic? I am imagining that we name all the resources based on the branch's name. If so, do we:
- Still need the random id? That is, if there's a branch 1 year ago named
foo
which has been merged intomaster
, and now I create a branchfoo
, will there be an issue? - Do resources need to have a globally unique name (i.e. unique across all resources created by everyone using Azure). If not, can we simply name the Resource Group with the branch's name, and all the resources inside the Resource Group do not use prefixes? (i.e. imagine a folder containing files - the files across the different folders can have the same 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.
- Storage account names have to be unique within Azure mentioned here, hence the use for random-id
When naming your storage account, keep these rules in mind:
Storage account names must be between 3 and 24 characters in length and may contain numbers and lowercase letters only.
Your storage account name must be unique within Azure. No two storage accounts can have the same name.
- Sure, will update names of resource groups to be branch names
terraform/test/main.tf
Outdated
|
||
backend "azurerm" { | ||
resource_group_name = "testResource" | ||
storage_account_name = "teststorage70757" |
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.
storage_account_name
here is hardcoded
terraform/init/variables.tf
Outdated
default = "southeastasia" | ||
description = "Location of the resource group." |
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.
It is not a required field. Can be removed since it's straightforward
terraform/init/variables.tf
Outdated
variable "prefix" { | ||
default = "test" | ||
description = "Prefix of the resource group name that's combined with a random ID so name is unique in your Azure subscription." | ||
} |
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.
- Storage account names have to be unique within Azure mentioned here, hence the use for random-id
When naming your storage account, keep these rules in mind:
Storage account names must be between 3 and 24 characters in length and may contain numbers and lowercase letters only.
Your storage account name must be unique within Azure. No two storage accounts can have the same name.
- Sure, will update names of resource groups to be branch names
|
||
- name: Terraform Init | ||
working-directory: ./terraform/terraform-remote-state | ||
run: terraform init |
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.
does the GitHub agent have terraform
installed as a command-line tool? You might need
- uses: hashicorp/setup-terraform@v1
Fixes #96