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

Add BPL_DEBUG_ENABLED to set --inspect #742

Merged

Conversation

c0d1ngm0nk3y
Copy link
Contributor

@c0d1ngm0nk3y c0d1ngm0nk3y commented Oct 10, 2023

fixes #741

Summary

Add support for BPL_DEBUG_ENABLED

We added another helper inspector. This helper check for BPL_DEBUG_ENABLED and BPL_DEBUG_PORT and adds the corresponding --inspect to the NODE_OPTIONS.

There are 2 main reasons why this pull request seems larger than expected at first sight:

  • We had to move some functionality from optimize-memory/internal in order to use it
  • Instead of adding just another testdata, we removed testdata/optimize-memory and adapted existing and our new test to use testdata/simple_app.

Use Cases

The same could already be achieved by setting NODE_OPTIONS directly, but to implement RFC 0037 it can be configured in the same way as for other buildpacks.

Checklist

  • I have viewed, signed, and submitted the Contributor License Agreement.
  • I have linked issue(s) that this PR should close using keywords or the Github UI (See docs)
  • I have added an integration test, if necessary.
  • I have reviewed the styleguide for guidance on my code quality.
  • I'm happy with the commit history on this PR (I have rebased/squashed as needed).

@c0d1ngm0nk3y c0d1ngm0nk3y requested a review from a team as a code owner October 10, 2023 12:38
@c0d1ngm0nk3y c0d1ngm0nk3y added the semver:minor A change requiring a minor version bump label Oct 10, 2023
@c0d1ngm0nk3y c0d1ngm0nk3y force-pushed the enable-remote-debug branch 3 times, most recently from 4b69041 to 24426c3 Compare October 13, 2023 07:53
@c0d1ngm0nk3y
Copy link
Contributor Author

@paketo-buildpacks/nodejs-maintainers Any comments/remarks? It add some convenience when using a debugger.

@loewenstein
Copy link

@paketo-buildpacks/nodejs-maintainers anyone? If I see it correctly, this would add another check on paketo-buildpacks/rfcs#175 which would be nice supposedly.

Signed-off-by: Ralf Pannemans <[email protected]>
Signed-off-by: Pavel Busko <[email protected]>
Co-authored-by: Ralf Pannemans <[email protected]>
@pbusko pbusko force-pushed the enable-remote-debug branch from a311946 to acc3d96 Compare December 11, 2023 09:10
@c0d1ngm0nk3y
Copy link
Contributor Author

@paketo-buildpacks/nodejs-maintainers Any chance of getting a review? The actual change is rather smal, but 2 refactorings make it a bit more to review:

  • Moving reused code from optimize-memory to utils
  • Reusing simple_app in integration tests instead of having a test app for each integration test

@loewenstein
Copy link

@paketo-buildpacks/nodejs-maintainers ^^

@thitch97
Copy link
Contributor

thitch97 commented Jan 4, 2024

Apologies for the delay here, folks. I will review this in short order.

@thitch97 thitch97 merged commit 8e2feaf into paketo-buildpacks:main Jan 16, 2024
10 checks passed
@modulo11 modulo11 deleted the enable-remote-debug branch January 17, 2024 08:37
@thitch97 thitch97 mentioned this pull request Feb 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver:minor A change requiring a minor version bump
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement RFC0037: Enable Remote Debug Support
3 participants