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.

Sign up for GitHub

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

Jump to bottom

Fix Default Route Table #62

Merged
merged 18 commits into from
Jun 28, 2022
Merged

Fix Default Route Table #62

merged 18 commits into from
Jun 28, 2022

Conversation

Benbentwo
Copy link
Member

@Benbentwo Benbentwo commented Jun 24, 2022 •
edited by nitrocode
Loading

@Benbentwo Benbentwo requested review from a team as code owners June 24, 2022 17:42
@Benbentwo Benbentwo requested review from adamcrews and milldr and removed request for a team June 24, 2022 17:42
@Benbentwo
Copy link
Member Author

/test all

aknysh
aknysh reviewed Jun 24, 2022
View reviewed changes
accepter.tf Outdated Show resolved Hide resolved
aknysh
aknysh reviewed Jun 24, 2022
View reviewed changes
accepter.tf Show resolved Hide resolved
aknysh
aknysh reviewed Jun 24, 2022
View reviewed changes
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

Benbentwo and others added 2 commits June 24, 2022 11:55
@Benbentwo @aknysh
Apply suggestions from code review
c60d53f 
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
@Benbentwo
revert accidental dleetion
8565edf
Nuru
Nuru previously requested changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

The aws_subnet_ids resource has been deprecated, and we should use aws_subnets instead.

Although aws_route_table has not been deprecated, we should probably use aws_route_tables instead, as it has the cleaner, more modern interface.

We can fix the original bug by looking up the default route table and using it if no route table is found to be associated with the subnet.

accepter.tf Outdated Show resolved Hide resolved
@mergify mergify bot dismissed Nuru’s stale review June 24, 2022 19:47

This Pull Request has been updated, so we're dismissing all reviews.

@Benbentwo
Copy link
Member Author

I believe I addressed your comments. using aws_subnet_ids now.

We we're already using aws_route_tables in the PR so nothing changed there. Tested with one using default route table, and one without.

@Benbentwo
Copy link
Member Author

/test all

Nuru
Nuru previously requested changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@Nuru Nuru left a comment

Choose a reason for hiding this comment

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

Backtracking on removing sort for data from data sources, since it seems to work. See other comments.

accepter.tf Show resolved Hide resolved
accepter.tf Outdated Show resolved Hide resolved
accepter.tf Outdated Show resolved Hide resolved
@mergify mergify bot dismissed Nuru’s stale review June 24, 2022 21:18

This Pull Request has been updated, so we're dismissing all reviews.

@Benbentwo
Copy link
Member Author

/test all

@Benbentwo
Copy link
Member Author

/test all

@Benbentwo
Copy link
Member Author

/test all

aknysh
aknysh reviewed Jun 28, 2022
View reviewed changes
accepter.tf Show resolved Hide resolved
aknysh
aknysh reviewed Jun 28, 2022
View reviewed changes
accepter.tf Outdated Show resolved Hide resolved
aknysh
aknysh reviewed Jun 28, 2022
View reviewed changes
accepter.tf Outdated Show resolved Hide resolved
aknysh
aknysh reviewed Jun 28, 2022
View reviewed changes
accepter.tf Outdated Show resolved Hide resolved
aknysh
aknysh reviewed Jun 28, 2022
View reviewed changes
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.

@Benbentwo a few nitpicks

Benbentwo and others added 3 commits June 27, 2022 17:39
@Benbentwo @aknysh
Apply suggestions from code review
2bbe5e4 
Co-authored-by: Andriy Knysh <aknysh@users.noreply.github.com>
@cloudpossebot
Auto Format
b11f41d
@Benbentwo
PR Comments
a4ffe3c
@Nuru
Copy link
Contributor

Nuru commented Jun 28, 2022

/test all

@Nuru Nuru self-requested a review June 28, 2022 02:14
Nuru
Nuru previously requested changes Jun 28, 2022
View reviewed changes
accepter.tf Outdated Show resolved Hide resolved
accepter.tf Show resolved Hide resolved
@Benbentwo @Nuru
Apply suggestions from code review
33f4710 
Co-authored-by: Nuru <Nuru@users.noreply.github.com>
@mergify mergify bot dismissed Nuru’s stale review June 28, 2022 02:28

This Pull Request has been updated, so we're dismissing all reviews.

@Nuru
Copy link
Contributor

Nuru commented Jun 28, 2022

/test all

@Benbentwo Benbentwo merged commit 33a24a3 into master Jun 28, 2022
@Benbentwo Benbentwo deleted the revert-pr branch June 28, 2022 02:38
@Benbentwo Benbentwo changed the title Revert PR #44 Fix Default Route Table Jun 28, 2022
@Gowiem Gowiem mentioned this pull request Jul 7, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aknysh aknysh aknysh approved these changes

@Nuru Nuru Nuru approved these changes

@adamcrews adamcrews Awaiting requested review from adamcrews adamcrews was automatically assigned from cloudposse/contributors

@milldr milldr Awaiting requested review from milldr milldr was automatically assigned from cloudposse/contributors

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Module fails if subnet uses default "main" routing table
4 participants
@Benbentwo @Nuru @aknysh @cloudpossebot