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

fix(*): patch expanduser to be more friendly to OpenResty environment #111

Merged
merged 5 commits into from
Apr 19, 2024

Conversation

windmgc
Copy link
Member

@windmgc windmgc commented Apr 16, 2024

Summary

pl.path.expanduser may return with nil, err when received a path argument starting with ~ but $HOME(or other home path related) environment variable is not present. This PR adds proper error handling code when calling expanduser.

Copy link

github-actions bot commented Apr 16, 2024

Luacheck Report

76 tests    0 ✅  0s ⏱️
 1 suites   0 💤
 1 files    76 ❌

For more details on these failures, see this check.

Results for commit d0f91c0.

♻️ This comment has been updated with latest results.

Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

Then userfriendly way to resolve this is to read the env vars in the config module along with the others (the AWS specific ones).

Probably best to copy the Penlight code here, and modify it to fall back to the cached variables.

Just ensure we also cater for Windows (despite we're not using it)

@windmgc
Copy link
Member Author

windmgc commented Apr 17, 2024

A bug has also been detected that expanded file path is not being used to read content, see:

local contents, err = pl_config.read(filename, { variabilize = false })

I fixed it in the latest commit

@windmgc windmgc requested a review from Tieske April 17, 2024 09:17
@windmgc windmgc changed the title fix(*): add error handling for expanduser fix(*): patch expanduser to be more friendly to OpenResty environment Apr 17, 2024
src/resty/aws/config.lua Outdated Show resolved Hide resolved
src/resty/aws/config.lua Outdated Show resolved Hide resolved
src/resty/aws/config.lua Outdated Show resolved Hide resolved
@windmgc
Copy link
Member Author

windmgc commented Apr 17, 2024

@Tieske Yes, I intended to not handle the error returned by these three functions as the expanduser error seems to be not fatal for these three functions(if it fails the function will return something else or follow other logic), but it is a fatal error for load_file, so I only handled the error in load_file.

In the last commit, I added some debug logs to these three functions in case we encounter error and have problem debugging them. Please review again! Thanks

@windmgc windmgc requested a review from Tieske April 18, 2024 02:44
Tieske
Tieske previously approved these changes Apr 18, 2024
@Tieske
Copy link
Member

Tieske commented Apr 18, 2024

BTW; does need a changelog entry!

@windmgc windmgc merged commit fad696b into main Apr 19, 2024
9 checks passed
@windmgc windmgc deleted the error-handling-expanduser branch April 19, 2024 05:23
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