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

convert route endpoints to new lookup API #839

Merged
merged 60 commits into from
Mar 31, 2022
Merged

Conversation

davepacheco
Copy link
Collaborator

This change updates the "route" endpoints to use the new lookup API.

As part of this, I changed the router_id column on the router_route table to be called vpc_router_id, so that we can maintain the expectation that the foreign key id is derivable from the foreign table name. #798 depends on this.

@davepacheco davepacheco mentioned this pull request Mar 31, 2022
71 tasks
@davepacheco
Copy link
Collaborator Author

I think this is ready to review but it's blocked on #798.

Copy link
Contributor

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

Looks great. Kind of funny that it's more code, but it's very nice code. And deleting vpc_router_lookup_by_path eventually will offset it.

@davepacheco
Copy link
Collaborator Author

Yeah, I'd forgotten to remove vpc_router_lookup_by_path. It's less code now!

@david-crespo
Copy link
Contributor

oh right, we can remove the hand-written database functions now

never mind about it being more code!

@zephraph
Copy link
Contributor

This is just sooo damn good.

Base automatically changed from new-lookup to main March 31, 2022 19:27
@davepacheco davepacheco marked this pull request as ready for review March 31, 2022 21:02
@davepacheco davepacheco enabled auto-merge (squash) March 31, 2022 21:03
@davepacheco davepacheco merged commit 79fb3b0 into main Mar 31, 2022
@davepacheco davepacheco deleted the convert-new-lookup branch March 31, 2022 21:39
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.

3 participants