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

feat(router) new router support and tests fix #8938

Merged
merged 1 commit into from
Jul 13, 2022
Merged

feat(router) new router support and tests fix #8938

merged 1 commit into from
Jul 13, 2022

Conversation

dndx
Copy link
Member

@dndx dndx commented Jun 13, 2022

Most tests working, still fixing a few leftover ones

@dndx dndx requested a review from a team as a code owner June 13, 2022 18:51
@dndx dndx marked this pull request as draft June 13, 2022 18:52
@mayocream mayocream added this to the 3.0 milestone Jun 13, 2022
local router = setmetatable({
schema = s,
router = inst,
routes = {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use table.new to pre-allocate table?

kong/router/atc_compat.lua Outdated Show resolved Hide resolved
kong/router/atc_compat.lua Outdated Show resolved Hide resolved
kong/router/atc_compat.lua Outdated Show resolved Hide resolved
kong/router/atc_compat.lua Show resolved Hide resolved
@dndx dndx force-pushed the feat/new_router branch 13 times, most recently from 515515d to f4869a0 Compare July 12, 2022 09:17
.github/workflows/build_and_test.yml Outdated Show resolved Hide resolved
.requirements Outdated
@@ -9,5 +9,5 @@ RESTY_PCRE_VERSION=8.45
RESTY_LMDB_VERSION=master
RESTY_EVENTS_VERSION=0.1.1
LIBYAML_VERSION=0.2.5
KONG_BUILD_TOOLS_VERSION=4.30.0
KONG_BUILD_TOOLS_VERSION=feat/atc_router_build
Copy link
Contributor

Choose a reason for hiding this comment

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

change it to official version before merge

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this could be 4.32.3 or 4.33.



local function is_regex_magic(path)
return sub(path, 1, 1) == "~"
Copy link
Contributor

Choose a reason for hiding this comment

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

It may be replaced with OP_REGEX below?

kong/router/atc_compat.lua Show resolved Hide resolved
@github-actions github-actions bot removed the chore Not part of the core functionality of kong, but still needed label Jul 13, 2022
@github-actions github-actions bot added the chore Not part of the core functionality of kong, but still needed label Jul 13, 2022
@dndx dndx marked this pull request as ready for review July 13, 2022 08:52
@github-actions github-actions bot removed the chore Not part of the core functionality of kong, but still needed label Jul 13, 2022
@dndx dndx merged commit 5045dd7 into master Jul 13, 2022
@dndx dndx deleted the feat/new_router branch July 13, 2022 22:04
Copy link
Contributor

@StarlightIbuki StarlightIbuki left a comment

Choose a reason for hiding this comment

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

We need to add ATC to .requirement.
Also, we need to add libatc_router.so to dyn lib search path (cpath).
Update: It's my local build's problem. I manually installed the ATC library on the wrong path.

And, maybe we need to update developer.md about passing ATC's version to build script.

@chronolaw
Copy link
Contributor

chronolaw commented Jul 14, 2022

We aslo need to add an entry in changelogs.md


local c = context.new(self.schema)

for _, field in ipairs(self.fields) do
Copy link
Contributor

Choose a reason for hiding this comment

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

it seems headers are the only variables that we fetch lazily. Could we just unconditionally add all other fields here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants