Skip to content

Add Route to RouteTable using ACK runtime v0.15.2 #21

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
Merged
merged 17 commits into from
Nov 18, 2021
Merged

Add Route to RouteTable using ACK runtime v0.15.2 #21

merged 17 commits into from
Nov 18, 2021

Conversation

brycahta
Copy link
Contributor

Issue #, if available: aws-controllers-k8s/community#489

Description of changes:

  • Moves route from Status to Spec so users can define Routes upon RouteTable creation
  • Changes to handle default route created by RouteTable & user-defined routes
  • Tests for route

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@brycahta brycahta requested a review from bwagner5 November 16, 2021 00:14
RedbackThomson
RedbackThomson reviewed Nov 16, 2021
View reviewed changes
Copy link
Contributor

@RedbackThomson RedbackThomson left a comment

Choose a reason for hiding this comment

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

Awesome use of the custom list fields - glad to see it came in handy. Left some inline comments, especially regarding removing generated code that you would otherwise need to maintain.

apis/v1alpha1/generator.yaml Outdated
Comment on lines 90 to 93
fields:
Routes:
custom_field:
list_of: Route
Copy link
Contributor

Choose a reason for hiding this comment

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

So clean ❤️

Copy link
Contributor

Choose a reason for hiding this comment

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

So fresh.

@brycahta
Copy link
Contributor Author

brycahta commented Nov 16, 2021 •
edited
Loading

Taking a look at failing Subnet test. Might be related to me adding a public subnet to the SharedVPC

Spoiler: it was "InvalidSubnet.Conflict: The CIDR '10.0.0.0/16' conflicts with another subnet

bwagner5
bwagner5 reviewed Nov 16, 2021
View reviewed changes
Copy link
Contributor

@bwagner5 bwagner5 left a comment

Choose a reason for hiding this comment

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

nice! A few comments and questions, feel free to take them or leave them

jaypipes
jaypipes reviewed Nov 16, 2021
View reviewed changes
apis/v1alpha1/generator.yaml Outdated
Comment on lines 90 to 93
fields:
Routes:
custom_field:
list_of: Route
Copy link
Contributor

Choose a reason for hiding this comment

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

So fresh.

jaypipes
jaypipes approved these changes Nov 17, 2021
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

All good with me. If @RedbackThomson and @bwagner5 want to send it on its merry way, that would be cool. :)

@brycahta brycahta requested a review from jaypipes November 17, 2021 22:55
jaypipes
jaypipes approved these changes Nov 18, 2021
View reviewed changes
Copy link
Contributor

@jaypipes jaypipes left a comment

Choose a reason for hiding this comment

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

Looks awesome, thanks @brycahta! @RedbackThomson feel free to send this on its merry way if you agree.

@brycahta
Merge pull request #1 from RedbackThomson/route-or-route
b23c5f3 
Generate create, delete and setter for Route
@RedbackThomson
Copy link
Contributor

/test ec2-kind-e2e

RedbackThomson
RedbackThomson approved these changes Nov 18, 2021
View reviewed changes
Copy link
Contributor

@RedbackThomson RedbackThomson left a comment

Choose a reason for hiding this comment

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

Awesome. This is in a great spot, now! I really don't see any compromises for users or for yourself as the maintainer.

Ship it!

/lgtm

pkg/resource/route_table/hooks.go
Comment on lines +154 to +156
// Routes should only be configurable for the
// RouteTable in which they are defined
input.RouteTableId = r.ko.Status.RouteTableID
Copy link
Contributor

Choose a reason for hiding this comment

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

+1

@ack-bot ack-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 18, 2021
@ack-bot
Copy link
Collaborator

ack-bot commented Nov 18, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brycahta, jaypipes, RedbackThomson

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:
  • OWNERS [RedbackThomson,brycahta,jaypipes]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ack-bot ack-bot merged commit c774f71 into aws-controllers-k8s:main Nov 18, 2021
@brycahta brycahta deleted the route-or-route branch November 18, 2021 21:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jaypipes jaypipes jaypipes approved these changes

@RedbackThomson RedbackThomson RedbackThomson approved these changes

@a-hilaly a-hilaly Awaiting requested review from a-hilaly

@bwagner5 bwagner5 Awaiting requested review from bwagner5

Assignees

@RedbackThomson RedbackThomson

Labels
approved lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brycahta @RedbackThomson @ack-bot @jaypipes @bwagner5