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

NH-89983: remove doc/ from lambda build; fix logging #152

Merged
merged 3 commits into from
Sep 10, 2024
Merged

Conversation

xuan-cao-swi
Copy link
Contributor

Description

Test (if applicable)

Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

Another note from testing--if the function handler code had something like:

require 'foo'
# require 'bar'
##require 'baz'

we should not try to preload the commented out libraries.

lambda/otel/layer/otel_wrapper.rb Outdated Show resolved Hide resolved
@xuan-cao-swi
Copy link
Contributor Author

The current regex scan will ignore the comment, for example

irb(main):002:0> File.read("sample_lambda_file.rb")
=> "require 'foo'\n# require 'bar'\n##require 'baz'\n"
irb(main):003:0> File.read("sample_lambda_file.rb").scan(/^\s*require\s+['"]([^'"]+)['"]/).flatten
=> ["foo"] # only the library name (e.g. foo) inside array will be called with require

@cheempz cheempz marked this pull request as ready for review September 6, 2024 22:52
@cheempz cheempz requested a review from a team as a code owner September 6, 2024 22:52
Copy link
Contributor

@cheempz cheempz left a comment

Choose a reason for hiding this comment

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

LGTM! added a commit to fix the docs dir removal--ran through the adhoc testing today with updated layer and looks good, let's get this PR in and we can talk next week on next steps for a GA release @xuan-cao-swi :)

@xuan-cao-swi xuan-cao-swi merged commit 72e1d58 into main Sep 10, 2024
18 checks passed
@xuan-cao-swi xuan-cao-swi deleted the NH-89983 branch September 11, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants