Skip to content

Conversation

ymorimo
Copy link
Contributor

@ymorimo ymorimo commented Sep 17, 2020

https://scalar-labs.atlassian.net/browse/DLT-7024

@ymorimo ymorimo self-assigned this Sep 17, 2020
@ymorimo ymorimo marked this pull request as draft September 17, 2020 09:56
@ymorimo
Copy link
Contributor Author

ymorimo commented Sep 17, 2020

This won't work until scalar-labs/scalar-terraform#208 is finished

@ymorimo ymorimo marked this pull request as ready for review September 24, 2020 03:20
name = data.terraform_remote_state.network.outputs.network_name
dns = data.terraform_remote_state.network.outputs.dns_zone_id
id = data.terraform_remote_state.network.outputs.network_id
region = data.terraform_remote_state.network.outputs.region
Copy link
Contributor

Choose a reason for hiding this comment

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

The following may not be necessary.

  • cidr
  • dns
  • id
  • bastion_ip
  • bastion_provision_id
  • internal_domain

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Thanks 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

@tei-k tei-k left a comment

Choose a reason for hiding this comment

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

LGTM! (left one comment)

Copy link
Collaborator

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

Looking good! I have a question.

@@ -0,0 +1,15 @@
data "terraform_remote_state" "network" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need remote.tf.azurerm for a case where blob storage is used for the backend?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In scalar-k8s, cassandra and monitor modules have remote.tf.azurerm (and remote.tf.s3 also), but kuberrnetes doesn't.
I wonder if these example files should be provided because they are all very similar and I think it's getting harder to keep them all correct and up-to-date. It might be better to document how to use the remote backend rather than providing example files.

Copy link
Collaborator

@feeblefakie feeblefakie Sep 30, 2020

Choose a reason for hiding this comment

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

Yes, but then backend.tf.azurerm is pretty similar to other backend files.
So that's probably another issue to fix; removing redundant files.
As for this, I think it should be consistent with the others.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR, I have added remote.tf.azurerm for the cosmosdb module.

But to be fully consistent, remote.tf.azurerm (and remote.tf.s3 also?) need to be added to kubernetes module as well. That's the same for backend.tf. And monitor/remote.tf.azurerm and monitor/remote.tf.s3 already look wrong when compared to module/remote.tf. I'll fix these issues later.

BTW, basically, should we have s3 backend files in azure modules? When taking a look at scalar-terraform, it looks like modules in examples/aws have only s3 versions, but in example/azure, some modules have only azurerm versions, some have both versions, and some don't have either. I think s3 files can be removed from azure examples.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!
I think we should remove s3 files from Azure.
So to sum up, the following needs to be fixed?

  • add remote.tf.azurerm for kubenetes module
  • fix monitor.tf.s3 and azurerm
  • remove s3 files from azure

I think it can be done in one PR or separate PRs.
Could you?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the summary! Sure!

Copy link
Collaborator

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks!

@@ -0,0 +1,15 @@
data "terraform_remote_state" "network" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks!
I think we should remove s3 files from Azure.
So to sum up, the following needs to be fixed?

  • add remote.tf.azurerm for kubenetes module
  • fix monitor.tf.s3 and azurerm
  • remove s3 files from azure

I think it can be done in one PR or separate PRs.
Could you?

@feeblefakie feeblefakie merged commit f3893d8 into master Sep 30, 2020
@feeblefakie feeblefakie deleted the cosmosdb-tf branch September 30, 2020 06:45
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.

3 participants