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

chore(*) Bundling proto files inside the luarock #8918

Closed
wants to merge 1 commit into from

Conversation

tyler-ball
Copy link
Contributor

@tyler-ball tyler-ball commented Jun 7, 2022

Summary

This allows us to incloud these source files as siblings to the rest of
our source code. It also allows us to look up the location of them
at runtime instead of hardcoding them into /usr/local/kong/include.

This does mean future proto files (or other includes) will need to get added to the .rockspec but I think that is better than hardcoding the protobuf lookup path.

Full changelog

  • Bundle our proto files inside the Kong luarock

Related

@@ -10,3 +10,4 @@ RESTY_LMDB_VERSION=master
LIBYAML_VERSION=0.2.5
KONG_BUILD_TOOLS_VERSION=4.30.0
KONG_NGINX_MODULE_BRANCH=0.2.1
PROTOBUF_VERSION=3.19.0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@@ -65,9 +66,12 @@ end

function _M.new()
local protoc_instance = protoc.new()
local kong_init_file, _, _ = loader.which("kong")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

loader.which returns the path to a specified module

kong-2.8.0-0.rockspec Outdated Show resolved Hide resolved
@tyler-ball tyler-ball requested a review from hishamhm June 7, 2022 21:43
it("includes kong/include in the load path", function()
local grpc_tools_instance = grpc_tools.new()
assert.matches("kong/include$", grpc_tools_instance.protoc_instance.paths[3])
end)
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a test to load the .proto file with pure luarocks loader.

Suggested change
end)
end)
it("loads .proto files with luarocks loader", function()
local loader = require "luarocks.loader"
local kong_init_file, _, _ = loader.which("kong")
local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")
local protoc = require "protoc"
local p = protoc.new()
p.include_imports = true
p:addpath(kong_include_dir)
assert(p:loadfile("opentelemetry/proto/collector/trace/v1/trace_service.proto"))
end)

kong/tools/grpc.lua Show resolved Hide resolved
@@ -65,9 +66,12 @@ end

function _M.new()
local protoc_instance = protoc.new()
local kong_init_file, _, _ = loader.which("kong")
local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")
Copy link
Member

@kikito kikito Jun 8, 2022

Choose a reason for hiding this comment

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

Overprotective code is not my cup of tea, but this worries me. Two concerns:

  • The GRPC module shouldn't "know" about kong internal structure (like that our "main luarock uses a file called init.lua")
  • If we for some reason decide to rename init.lua to something else (unlikely, but it might happen if we decide to change how we load modules in the far future), this will fail with an obscure message.

My first impulse would be moving this logic to a function in kong.tools.utils.get_include_module and also write a test for it, so it breaks the tests if we change things. However I believe the grpc module gets required before utils, so this might not be possible. I don't know where else I would put this function. Kong.global or even kong.constants, perhaps? The path shouldn't change once kong starts after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • The GRPC module needs to know that from the root of the installed luarock (or source code directory), there is a folder named kong/include that stores all the proto files to be loaded. I agree that looking for kong/init.lua isn't the best approach but there isn't a luarock API right now for "give me the install root of a rock", only "give me the path to a file in a luarock").
  • I'm against moving tiny pieces of code that are only used once into utility modules. I think that just obfuscates the code and makes it harder to read. I did add a new comment here to (hopefully) better explain why I am calling the current API.
  • I think if we renamed kong/init.lua the tests I wrote would break trying to call :gsub on a nil table. I'm open to adding an assert with a better message like assert(kong_init_file, "Could not find the 'init.lua' file in the root of the kong luarock"). Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Let's go with the assert then.

Suggested change
local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")
local kong_include_dir = kong_init_file:gsub("init%.lua$", "include")
assert(kong_include_dir, "Could not find the 'init.lua' file in the root of the kong luarock")

This allows us to incloud these source files as siblings to the rest of
our source code. It also allows us to look up the location of them
at runtime instead of hardcoding them into /usr/local/kong/include.

Signed-off-by: Tyler Ball <[email protected]>
@Tieske
Copy link
Member

Tieske commented Oct 3, 2022

drive by comment:

have we been been looking at https://github.com/hishamhm/datafile for this functionality?

It allows to load files included in a rockspec like this: https://github.com/hishamhm/datafile/blob/78efd1f9de2c15a00c7d7c73ea71684c0d50a2f4/test/test-datafile-scm-1.rockspec#L24-L26

@hbagdi
Copy link
Member

hbagdi commented Mar 20, 2023

Closing this due to lack of activity. Please re-open if needed.

@hbagdi hbagdi closed this Mar 20, 2023
@hbagdi hbagdi deleted the feat/proto_bundling branch March 20, 2023 22:51
@hanshuebner hanshuebner restored the feat/proto_bundling branch March 21, 2023 15:42
@hbagdi hbagdi deleted the feat/proto_bundling branch March 22, 2023 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants