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

Store install report in extension directory #550

Closed
2 tasks done
tombruijn opened this issue Jan 17, 2022 · 0 comments · Fixed by #553
Closed
2 tasks done

Store install report in extension directory #550

tombruijn opened this issue Jan 17, 2022 · 0 comments · Fixed by #553
Assignees
Labels

Comments

@tombruijn
Copy link
Member

tombruijn commented Jan 17, 2022

The Node.js package now writes the install report in the system tmp directory. This was necessary because the nodejs-ext package would not persist on the file system in case of an installation failure. In PR #545 we've merged the nodejs-ext package in the main nodejs package, and it will no longer fail on install. Even if the installation fails the install files persist on the file system.

We need to change the install report location from /tmp/appsignal-<generated key>-install.report to the location in the ext dir in the nodejs package. The new report file can be named install.report like in the other integrations.

To do

  • Move the install report to the nodejs/ext directory.
  • Rename the install report file to install.report.

Be aware that the paths need to be updated for the installer script that writes the file and the diagnose tool that reads it.

tombruijn added a commit that referenced this issue Jan 18, 2022
In PR #545 we've merged the nodejs-ext package into the nodejs package.
This change moved some files around, and moved the extension install
scripts one level deeper into the `extension` subdirectory. Update the
report path name generation to match the new depth of the file.

Previously: `packages/nodejs-ext/scripts/extension.js`
New: `packages/nodejs/scripts/extension/extension.js`

It's interesting that no tests failed because of this. I think it's not
caught in the diagnose_tests, because we stub the install report.

I've now added a simple test to test the presence of the report, and not
a parsing error that occurs when no file could be read or there was a
problem reading the file. I'm struggling with Jest's mock system to
write a test for this and I'd rather look at it when addressing issue
#550 that updates the path location, which makes it easier to stub I
hope.
@tombruijn tombruijn self-assigned this Jan 18, 2022
tombruijn added a commit that referenced this issue Jan 18, 2022
In PR #545 we've merged the nodejs-ext package into the nodejs package.
It no longer fails the package install on extension installation
failure, which means the ext directory remains after a installation
failure.

We can now store the install report file in the package's `ext`
directory, like we do in all the other integrations. This means we no
longer need to generate a filename for the install report to store it in
the system tmp directory.

Closes #550
tombruijn added a commit that referenced this issue Jan 18, 2022
In PR #545 we've merged the nodejs-ext package into the nodejs package.
It no longer fails the package install on extension installation
failure, which means the ext directory remains after a installation
failure.

We can now store the install report file in the package's `ext`
directory, like we do in all the other integrations. This means we no
longer need to generate a filename for the install report to store it in
the system tmp directory.

Closes #550
tombruijn added a commit that referenced this issue Jan 18, 2022
In PR #545 we've merged the nodejs-ext package into the nodejs package.
It no longer fails the package install on extension installation
failure, which means the ext directory remains after a installation
failure.

We can now store the install report file in the package's `ext`
directory, like we do in all the other integrations. This means we no
longer need to generate a filename for the install report to store it in
the system tmp directory.

Closes #550
tombruijn added a commit that referenced this issue Jan 18, 2022
In PR #545 we've merged the nodejs-ext package into the nodejs package.
It no longer fails the package install on extension installation
failure, which means the ext directory remains after a installation
failure.

We can now store the install report file in the package's `ext`
directory, like we do in all the other integrations. This means we no
longer need to generate a filename for the install report to store it in
the system tmp directory.

Closes #550
tombruijn added a commit that referenced this issue Jan 18, 2022
In PR #545 we've merged the nodejs-ext package into the nodejs package.
It no longer fails the package install on extension installation
failure, which means the ext directory remains after a installation
failure.

We can now store the install report file in the package's `ext`
directory, like we do in all the other integrations. This means we no
longer need to generate a filename for the install report to store it in
the system tmp directory.

Closes #550
tombruijn added a commit that referenced this issue Jan 19, 2022
In PR #545 we've merged the nodejs-ext package into the nodejs package.
It no longer fails the package install on extension installation
failure, which means the ext directory remains after a installation
failure.

We can now store the install report file in the package's `ext`
directory, like we do in all the other integrations. This means we no
longer need to generate a filename for the install report to store it in
the system tmp directory.

Closes #550
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 a pull request may close this issue.

1 participant