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

Integrate with Terraform #118

Open
wants to merge 26 commits into
base: master
Choose a base branch
from
Open

Integrate with Terraform #118

wants to merge 26 commits into from

Conversation

gohyk
Copy link
Collaborator

@gohyk gohyk commented Feb 4, 2022

This pull request will only add terraform files required to create the resources. Integration with pipeline will be done in another phase.

@gohyk gohyk requested a review from Zhiyuan-Amos February 4, 2022 03:44
@gohyk gohyk self-assigned this Feb 4, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Feb 4, 2022

Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-island-0901fe000-118.eastasia.azurestaticapps.net

1 similar comment
@github-actions
Copy link
Contributor

Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-island-0901fe000-118.eastasia.azurestaticapps.net

@gohyk gohyk force-pushed the gohyk-terraform-add branch from d744beb to 2699abb Compare February 27, 2022 10:30
@github-actions
Copy link
Contributor

github-actions bot commented Mar 1, 2022

Azure Static Web Apps: Your stage site is ready! Visit it here: https://orange-island-0901fe000-118.eastasia.azurestaticapps.net

@Zhiyuan-Amos Zhiyuan-Amos force-pushed the master branch 2 times, most recently from 331ade3 to d7fa9e3 Compare April 19, 2022 07:55
@gohyk gohyk force-pushed the gohyk-terraform-add branch from 431bd04 to 63eaa83 Compare May 7, 2022 17:56
@Zhiyuan-Amos Zhiyuan-Amos force-pushed the master branch 7 times, most recently from 49259a9 to 2e83a7a Compare May 10, 2022 08:33
@gohyk gohyk force-pushed the gohyk-terraform-add branch from dec3c1d to 9256501 Compare May 11, 2022 14:44
@gohyk gohyk marked this pull request as ready for review July 14, 2022 02:32
Copy link
Owner

@Zhiyuan-Amos Zhiyuan-Amos left a comment

Choose a reason for hiding this comment

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

  1. Will you be documenting down what are some of these files doing and how they are related to each other? For e.g. I see 2 .terraform.lock.hcl but I'm not sure if they are required.

  2. Typically what's the project structure like? For e.g. I've seen projects whereby the parent directory is tf instead of terraform. I'm not too particular about it, though it'll be good if we can set it to a default name.

Overall, this looks clean! 👍 I think the next best thing we can do now is to start documenting how this entire thing works. For a newb like myself, reading the code seems like magic.

Comment on lines +378 to +400
# Crash log files
crash.log

# Exclude all .tfvars files, which are likely to contain sentitive data, such as
# password, private keys, and other secrets. These should not be part of version
# control as they are data points which are potentially sensitive and subject
# to change depending on the environment.
#
*.tfvars

# Ignore override files as they are usually used to override resources locally and so
# are not checked in
override.tf
override.tf.json
*_override.tf
*_override.tf.json

# Include tfplan files to ignore the plan output of command: terraform plan -out=tfplan
*tfplan*

# Ignore CLI configuration files
.terraformrc
terraform.rc
Copy link
Owner

Choose a reason for hiding this comment

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

I understand you referenced this from an online source, but I'm wondering if it seems like there's some inconsistency in the comments e.g. some start with Ignore, Exclude and Include.

resource_group_name = azurerm_resource_group.rg.name

capabilities {
name = "EnableServerless"
Copy link
Owner

Choose a reason for hiding this comment

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

incorrect indentation I think.

Also, could document down what IDE you use to work with Terraform files (VS Code with some plugin installed?)

@@ -0,0 +1,43 @@
# azurerm_cosmosdb_account.db:
Copy link
Owner

Choose a reason for hiding this comment

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

Is this comment required?

Comment on lines 6 to 7
# Only one free tier cosmosDB per subscription
# enable_free_tier = true
Copy link
Owner

Choose a reason for hiding this comment

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

good comment

zone_redundant = false
}

timeouts {}
Copy link
Owner

Choose a reason for hiding this comment

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

are empty tags required, or can this entire line be removed?

Comment on lines 18 to 19
resource "random_uuid" "test" {
}
Copy link
Owner

Choose a reason for hiding this comment

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

does the auto-indentation cause this statement to take up 2 lines? Just checking

Comment on lines 9 to 10
default = "southeastasia"
description = "Location of the resource group."
Copy link
Owner

Choose a reason for hiding this comment

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

indentation of = is off

@@ -0,0 +1,11 @@
variable "prefix" {
default = "test"
Copy link
Owner

Choose a reason for hiding this comment

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

indentation of = looks weird

variable "prefix" {
default = "test"
}

Copy link
Owner

Choose a reason for hiding this comment

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

additional new line

Comment on lines +1 to +2
# Shortnames for regions can be found here:
# https://github.com/claranet/terraform-azurerm-regions/blob/master/REGIONS.md
Copy link
Owner

Choose a reason for hiding this comment

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

I notice this being repeated in the other variables.tf file as well. I wonder if this should be documented in a centralised location instead.

@gohyk
Copy link
Collaborator Author

gohyk commented Jul 20, 2022

  1. Will you be documenting down what are some of these files doing and how they are related to each other? For e.g. I see 2 .terraform.lock.hcl but I'm not sure if they are required.

I will write up a documenting a general guide on terraform as a README/Wiki for this. As for .terraform.lock.hcl I shall include it in .gitignore file. This file may cause issues when running terraform locally on different OS. The discussion can be found here: https://stackoverflow.com/questions/67963719/should-terraform-lock-hcl-be-included-in-the-gitignore-file

@Zhiyuan-Amos
Copy link
Owner

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.

2 participants