search

grem11n / terraform-aws-vpc-peering

Terraform module to setup VPC peering connection
https://registry.terraform.io/modules/grem11n/vpc-peering/aws/latest
Apache License 2.0
126 stars 91 forks source link

use subnets when peering #54

Closed tomasbackman closed 4 years ago

tomasbackman commented 4 years ago

I have added possibility for setting specific subnets when peering, for both this and peer vpc. Since we want this feature in our peering, (and cross account) and this was the mpt fitting module I could find =)

However I have tot been able to run the terratests at all localy, and have not done an example yet either. But can do so.

This is a draft PR so far for discussions around this =)

tomasbackman commented 4 years ago

So @grem11n could you help out with the tests for this feature addition? And also what do you think about this? Do you have any questions or agree/disagree on its inclusion?

grem11n commented 4 years ago

Hello @tomasbackman Thank you for contribution! Unfortunatelly, Terratest relies on my AWS creds, so it fails for forked repos.

I'll run them locally

grem11n commented 4 years ago

Looks good:

TestPeeringActive/MultiAccountMultiRegion 2020-04-29T14:47:46+02:00 command.go:158: Destroy complete! Resources: 12 destroyed.
--- PASS: TestPeeringActive (514.85s)
    --- PASS: TestPeeringActive/SingleAccountSingleRegion (79.49s)
    --- PASS: TestPeeringActive/SingleAccountSingleRegionWithOptions (79.36s)
    --- PASS: TestPeeringActive/SingleAccountMultiRegion (98.72s)
    --- PASS: TestPeeringActive/MultiAccountMultiRegion (257.29s)
PASS
ok      github.com/grem11n/terraform-aws-vpc-peering/test   514.866s
Time: 0h:08m:40s
❯ git status
On branch subnets-for-peering
Your branch is up to date with 'origin/subnets-for-peering'.
grem11n commented 4 years ago

@tomasbackman, do you think it makes sense to create test cases with pre-set subnets?

tomasbackman commented 4 years ago

@grem11n Hi. Yes I think that can be good, if nothing else as example. I have done it in our module where Im currently using my fork.

When running things locally I ran into som formatting things I had forgotten to push and did a rebase + force push (bad habit of mine) which affected this PR as well (forgot to change branch) But it should not have changed any functionality

tomasbackman commented 4 years ago

When running things locally I ran into som formatting things I had forgotten to push and did a rebase + force push (bad habit of mine) which affected this PR as well (forgot to change branch) But it should not have changed any functionality

But better test again to be safe.. sorry

I Marked this ready for review now.

Should I add an example using the subnets or will you? (I can not run terratest but should be able to put together an example similar to yours and run it locally at least)

grem11n commented 4 years ago

Ran tests once again just to be sure

TestPeeringActive/MultiAccountMultiRegion 2020-04-29T15:59:29+02:00 command.go:158: Destroy complete! Resources: 12 destroyed.
--- PASS: TestPeeringActive (355.88s)
    --- PASS: TestPeeringActive/SingleAccountSingleRegion (78.69s)
    --- PASS: TestPeeringActive/SingleAccountSingleRegionWithOptions (78.54s)
    --- PASS: TestPeeringActive/SingleAccountMultiRegion (99.58s)
    --- PASS: TestPeeringActive/MultiAccountMultiRegion (99.08s)
PASS
ok      github.com/grem11n/terraform-aws-vpc-peering/test   355.898s
grem11n commented 4 years ago

Should I add an example using the subnets or will you?

If you have AWS accounts to run the tests, I would appreciate that. Otherwise, I'll add these test cases. However, I won't promise any ETA on that