Skip to content
This repository has been archived by the owner on Jul 11, 2023. It is now read-only.

Module/single-port-sg: Support source_security_group #307

Merged
merged 1 commit into from
Apr 14, 2020
Merged

Conversation

Magicloud
Copy link
Contributor


name: Pull request template
about: Make a PR to terraform-aws-foundation

Please include the following in your PR:

Please also note that these are not hard requirements, but merely serve to define
what maintainers are looking for in PR's. Including these will more likely lead
to your PR being reviewed and accepted.

  • Update the changelog
  • Make sure that modules and files are documented. This can be done inside the module and files.
  • Make sure that new modules directories contain a basic README.md file.
  • Make sure that the module is added to tests/main.tf
  • Make sure that the linting passes on CI.
  • Make sure that there is an up to date example for your code:
    - For new modules this would entail example code for how to use the module or some explanation in the module readme.
    - For new examples please provide a README explaining how to run the example. It's also ideal to provide a basic makefile to use the example as well.
  • Make sure that there is a manual CI trigger that can test the deployment.

@Magicloud Magicloud mentioned this pull request Mar 4, 2020
7 tasks
@mcgirr
Copy link
Contributor

mcgirr commented Mar 5, 2020

Is there a way to use this without a targeted apply? I'm running into this situation that I think is stemming from the changes in the PR:

count                    = local.udp * local.by_src_sg

The "count" value depends on resource attributes that cannot be determined
until apply, so Terraform cannot predict how many instances will be created.
To work around this, use the -target argument to first apply only the
resources that the count depends on.

@Magicloud
Copy link
Contributor Author

Magicloud commented Mar 6, 2020

Is there a way to use this without a targeted apply? I'm running into this situation that I think is stemming from the changes in the PR:

count                    = local.udp * local.by_src_sg

The "count" value depends on resource attributes that cannot be determined
until apply, so Terraform cannot predict how many instances will be created.
To work around this, use the -target argument to first apply only the
resources that the count depends on.

I feel like this is a dep issue. I got this while destroying resources again. The source SG was gone (and this rule), but TF says it cannot calculate the count.

The reason I do it this way is that, source_security_group and cidr_blocks cannot be set at the same time, even one is an empty value.

@ketzacoatl Do you have any better idea? If not, I'd have to cancel this change.

Copy link
Contributor

@ketzacoatl ketzacoatl left a comment

Choose a reason for hiding this comment

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

Overall, this LGTM, I'd just like to separate the "enable" logic to locals so that is more obvious and harder to make a bug in the future.

modules/single-port-sg/main.tf Outdated Show resolved Hide resolved
@ketzacoatl
Copy link
Contributor

@Magicloud please update the above, TY!

This is a fork version of single-port-sg module to support source_security_group.
@Magicloud
Copy link
Contributor Author

Updated new design. Tested in PR #304.

Copy link
Contributor

@ketzacoatl ketzacoatl left a comment

Choose a reason for hiding this comment

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

As a separate module, this LGTM!

cc @mcgirr / @Magicloud do we have any branch references to update?

@ketzacoatl ketzacoatl merged commit b2032c1 into master Apr 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants