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

Adding external ENIs #159

Merged
merged 36 commits into from Jun 7, 2023
Merged

Adding external ENIs #159

merged 36 commits into from Jun 7, 2023

Conversation

jamengual
Copy link
Contributor

what

Add the ability to add external ENIs to the instance

why

For ec2 instances deployments clusters where the IPs are required to be unique and not change it is necessary to create ENIs outside of this module so that the instance termination does not change the ENI and IP attached.

references

@jamengual jamengual requested review from a team as code owners May 31, 2023 00:01
Copy link

@bridgecrew bridgecrew bot left a comment

Choose a reason for hiding this comment

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

Bridgecrew has found errors in this PR ⬇️

main.tf Show resolved Hide resolved
@jamengual
Copy link
Contributor Author

/terratest

main.tf Show resolved Hide resolved
@jamengual
Copy link
Contributor Author

/terratest

examples/external-eni/main.tf Outdated Show resolved Hide resolved
examples/external-eni/main.tf Outdated Show resolved Hide resolved
examples/external-eni/main.tf Outdated Show resolved Hide resolved
examples/external-eni/main.tf Outdated Show resolved Hide resolved
examples/external-eni/main.tf Outdated Show resolved Hide resolved
examples/external-eni/main.tf Outdated Show resolved Hide resolved
examples/external-eni/main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
main.tf Outdated Show resolved Hide resolved
jamengual and others added 4 commits May 30, 2023 22:36
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
examples/external-eni/main.tf Outdated Show resolved Hide resolved
examples/external-eni/main.tf Outdated Show resolved Hide resolved
examples/external-eni/main.tf Outdated Show resolved Hide resolved
jamengual and others added 3 commits May 30, 2023 22:36
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
main.tf Outdated Show resolved Hide resolved
main.tf Show resolved Hide resolved
outputs.tf Show resolved Hide resolved
Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

@jamengual thanks for the PR, please see comments.

Also, in the file https://github.com/cloudposse/terraform-aws-ec2-instance/blob/main/test/src/Makefile

we have

## Run tests
test: init
	go mod download
	go test -v -timeout 60m -run TestExamplesComplete

this will run only one test TestExamplesComplete and will not run the test that you added.

To run the test that you added, please do the following:

  1. Add your test here https://github.com/cloudposse/terraform-aws-ec2-instance/blob/main/test/src/examples_complete_test.go as a separate Go function
  2. Update this
## Run tests
test: init
	go mod download
	go test -v -timeout 60m -run TestExamplesComplete

to this

## Run tests
test: init
	go mod download
	go test -v -timeout 60m

This will run all the Go tests.

Thanks you

jamengual and others added 2 commits June 6, 2023 09:19
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
@jamengual
Copy link
Contributor Author

/terratest

@jamengual
Copy link
Contributor Author

/terratest

@jamengual
Copy link
Contributor Author

/terratest

@jamengual
Copy link
Contributor Author

/terratest

main.tf Show resolved Hide resolved
@jamengual
Copy link
Contributor Author

/terratest

outputs.tf Outdated
@@ -73,7 +73,7 @@ output "additional_eni_ids" {

output "ebs_ids" {
description = "IDs of EBSs"
value = aws_ebs_volume.default[*].id
value = one(aws_ebs_volume.default[*].id)
Copy link
Member

Choose a reason for hiding this comment

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

this aws_ebs_volume.default[*].id was correct since the output name is ebs_ids and we want a list of EIPs

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

please see comments

@jamengual
Copy link
Contributor Author

/terratest

outputs.tf Outdated Show resolved Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
@jamengual
Copy link
Contributor Author

/terratest

@jamengual
Copy link
Contributor Author

/terratest

@jamengual
Copy link
Contributor Author

I can't see the bridecrew issue but I though they were all addressed

Copy link
Member

@aknysh aknysh left a comment

Choose a reason for hiding this comment

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

thanks @jamengual

@aknysh aknysh merged commit f7457a5 into main Jun 7, 2023
10 of 11 checks passed
@aknysh aknysh deleted the add_external_eni branch June 7, 2023 03:31
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.

None yet

3 participants