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 extension installation reporting #409

Merged
merged 1 commit into from
Jul 9, 2021
Merged

Conversation

tombruijn
Copy link
Member

When the extension installed we never stored if it installed correctly
or not. Originally it would always report it as a "success" status. In a
previous commit cb08b7a, PR #373, I
changed the report status to "unknown", because after the extension.js
script was run the node-gyp rebuild command was run through the
package.json install script. After the node-gyp command was run it
would not return to the extension.js script and we couldn't check the
exit status of that command.

Instead of running the node-gyp command after the extension.js script,
run the node-gyp command from the extension.js script so we can listen
to the exit status and report the installation as a success or "error"
when it encounters an error.

This should fix #371, the issue describing the scenario above, the
install report not accurately describing the installation status.

I am not sure why the originally implementation was chosen. The original
implementation was present since the initial commit of the project.
There may be a problem with this new implementation that I am unaware of
now, because I'm no Node.js expert. But we call out to other commands
like tar from the extension.js script as well, so I figure it will
be fine.

CI build update

I've added tests for the extension build failure scenario, and the
success scenario. For this I needed to have Jest run certain specs only
some of the time, based on the _TEST_APPSIGNAL_EXTENSION_FAILURE
environment variable (same as in the Ruby gem CI setup). This is done
with the --filter option (which links to a file with a filter
function), which is configured through the package.json test scripts.

Since these new tests are only for the nodejs-ext package I found it
unnecessary to spin up a new job for it, and instead add it as
extra_commands to the package's test commands.

Before the failure state is tested, the extension installation is
cleaned up, so it doesn't think it has installed correctly.

Because we install the extension in the "Build" task, and it's not
present in the project dir, but instead the /tmp dir on the system,
I've also copied the report location from the "Build" task to the job
that tests this report. Otherwise it would fail on a missing file, or
we'd need to install the extension again. Hopefully we can find a better
location for the install report location in issue #372.

@tombruijn tombruijn added the bug label Jul 8, 2021
@tombruijn tombruijn self-assigned this Jul 8, 2021
@tombruijn tombruijn force-pushed the report-install-result branch 3 times, most recently from f7567d4 to 7565e9c Compare July 8, 2021 08:14
@tombruijn tombruijn marked this pull request as ready for review July 8, 2021 08:20
@@ -21,9 +21,15 @@ const {
} = require("./report")

const EXT_PATH = path.join(__dirname, "/../ext/")
const testExtensionFailure =
process.env._TEST_APPSIGNAL_EXTENSION_FAILURE == "true"
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
process.env._TEST_APPSIGNAL_EXTENSION_FAILURE == "true"
process.env._TEST_APPSIGNAL_EXTENSION_FAILURE === "true"

"use strict"

const testExtensionFailure =
process.env._TEST_APPSIGNAL_EXTENSION_FAILURE == "true"
Copy link
Member Author

Choose a reason for hiding this comment

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

Suggested change
process.env._TEST_APPSIGNAL_EXTENSION_FAILURE == "true"
process.env._TEST_APPSIGNAL_EXTENSION_FAILURE === "true"

When the extension installed we never stored if it installed correctly
or not. Originally it would always report it as a "success" status. In a
previous commit cb08b7a, PR #373, I
changed the report status to "unknown", because after the `extension.js`
script was run the `node-gyp rebuild` command was run through the
`package.json` `install` script. After the node-gyp command was run it
would not return to the extension.js script and we couldn't check the
exit status of that command.

Instead of running the node-gyp command after the `extension.js` script,
run the node-gyp command from the `extension.js` script so we can listen
to the exit status and report the installation as a success or "error"
when it encounters an error.

This should fix #371, the issue describing the scenario above, the
install report not accurately describing the installation status.

I am not sure why the originally implementation was chosen. The original
implementation was present since the initial commit of the project.
There may be a problem with this new implementation that I am unaware of
now, because I'm no Node.js expert. But we call out to other commands
like `tar` from the `extension.js` script as well, so I figure it will
be fine.

## CI build update

I've added tests for the extension build failure scenario, and the
success scenario. For this I needed to have Jest run certain specs only
some of the time, based on the `_TEST_APPSIGNAL_EXTENSION_FAILURE`
environment variable (same as in the Ruby gem CI setup). This is done
with the `--filter` option (which links to a file with a filter
function), which is configured through the `package.json` test scripts.

Since these new tests are only for the `nodejs-ext` package I found it
unnecessary to spin up a new job for it, and instead add it as
`extra_commands` to the package's test commands.

Before the failure state is tested, the extension installation is
cleaned up, so it doesn't think it has installed correctly.

Because we install the extension in the "Build" task, and it's not
present in the project dir, but instead the `/tmp` dir on the system,
I've also copied the report location from the "Build" task to the job
that tests this report. Otherwise it would fail on a missing file, or
we'd need to install the extension again. Hopefully we can find a better
location for the install report location in issue #372.
@tombruijn tombruijn force-pushed the report-install-result branch from 7565e9c to 249149d Compare July 9, 2021 09:26
@tombruijn tombruijn merged commit 1b54698 into main Jul 9, 2021
tombruijn added a commit that referenced this pull request Oct 7, 2021
The installation reuslt was hardcoded to always be `success`, because
previously we couldn't track it. But since PR #409 we now run the
`node-gyp` command from the `extension.js` install script so we can
track the installation result.

Read the status from the report so the diagnose report is more accurate.

Since there are more fields in case of an error or failure, print those
as well if present.
tombruijn added a commit that referenced this pull request Oct 7, 2021
The installation result was hardcoded to always be `success`, because
previously we couldn't track it. But since PR #409 we now run the
`node-gyp` command from the `extension.js` install script so we can
track the installation result.

Read the status from the report so the diagnose report is more accurate.

Since there are more fields in case of an error or failure, print those
as well if present.
tombruijn added a commit that referenced this pull request Oct 11, 2021
The installation result was hardcoded to always be `success`, because
previously we couldn't track it. But since PR #409 we now run the
`node-gyp` command from the `extension.js` install script so we can
track the installation result.

Read the status from the report so the diagnose report is more accurate.

Since there are more fields in case of an error or failure, print those
as well if present.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Diagnose report successfull install on download, not on installation
2 participants