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

Use node wrapper to debug NodeJS apps #4086

Merged
merged 5 commits into from
May 5, 2020

Conversation

briandealwis
Copy link
Member

@briandealwis briandealwis commented May 3, 2020

Fixes: #2170

Description

Cause NodeJS apps to use the newly-introduced node wrapper.

Many applications use NodeJS-based tools as part of their launch, like npm, rather than
invoke node directly. These intermediate node instances may interpret the
--inspect arguments. GoogleContainerTools/container-debug-support#34 introduced a node wrapper that only passes --inspect when provided an application script, and otherwise propagates --inspect via an environment variable.

One other minor change: this now sorts the environment variables when populating the k8s Container objects to make comparisons stable in the tests.

User facing changes

NodeJS apps that previously seemed undebuggable should now respond to debugging treatment.

@codecov
Copy link

codecov bot commented May 3, 2020

Codecov Report

Merging #4086 into master will increase coverage by 0.02%.
The diff coverage is 81.25%.

Impacted Files Coverage Δ
pkg/skaffold/debug/transform_nodejs.go 78.63% <81.25%> (+0.56%) ⬆️
...affold/kubernetes/portforward/kubectl_forwarder.go 68.29% <0.00%> (+2.43%) ⬆️

Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

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

so if I understand correctly, /dbg/nodejs/bin was added to the debug helper image in GoogleContainerTools/container-debug-support#34. but that image tag wasn't bumped anywhere here - where is that referenced in skaffold? i'm wondering if we integration tests that run in CI to make sure nothing in this image changes out from under our feet.

@briandealwis
Copy link
Member Author

We live at latest. There are tests in integration/testdata/debug for nodejs and npm. They wait for the deployments to be stable. Hmm, I should define some readiness probes there.

@dgageot dgageot merged commit 21061cd into GoogleContainerTools:master May 5, 2020
@briandealwis briandealwis deleted the nodewrapper branch May 16, 2020 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

skaffold debug: support npm-nodemon
4 participants