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

lua package for yaml support #6545

Merged
merged 1 commit into from
Sep 5, 2023
Merged

lua package for yaml support #6545

merged 1 commit into from
Sep 5, 2023

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Sep 4, 2023

Close #6544

@nopcoder nopcoder added new-feature Issues that introduce new feature or capability area/hooks improvements or additions to the hooks subsystem labels Sep 4, 2023
@nopcoder nopcoder self-assigned this Sep 4, 2023
@nopcoder nopcoder requested a review from Isan-Rivkin September 4, 2023 19:48
@nopcoder nopcoder added the include-changelog PR description should be included in next release changelog label Sep 4, 2023
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Neat!

Can you just add a note about how to test this manually?

func check(l *lua.State, err error) {
if err != nil {
lua.Errorf(l, err.Error())
panic("unreachable")
Copy link
Contributor

Choose a reason for hiding this comment

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

How expensive is this? And... are we sure we are exception-safe and defer x.Close() for all our Closers along the stack?

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 lua implementation we are using implemented the error handling using raise (panic), as described https://pkg.go.dev/github.com/Shopify/go-lua#Errorf.

Can go over the current provided files and lookup for resource management issues.

@nopcoder
Copy link
Contributor Author

nopcoder commented Sep 5, 2023

Neat!

Can you just add a note about how to test this manually?

  1. Thanks for the review!
  2. Sure - I've used the lakefs lua run capability with the following code:
local yaml = require("encoding/yaml")

content = [[
config:
    endpoint: http://localhost:8080
]]

local doc = yaml.unmarshal(content)
print("endpoint: ".. doc.config.endpoint)

local content2 = yaml.marshal(doc)
print("----")
print(content2)
print("----")
if content == content2 then
    print("success :)")
else
    print("failed!")
end

@nopcoder nopcoder merged commit 22ebf5f into master Sep 5, 2023
@nopcoder nopcoder deleted the feature/lua-yaml branch September 5, 2023 06:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/hooks improvements or additions to the hooks subsystem include-changelog PR description should be included in next release changelog new-feature Issues that introduce new feature or capability
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support yaml parser in Lua hooks
2 participants