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 the presence of the client file to diagnose #884

Merged
merged 1 commit into from
May 10, 2023

Conversation

luismiramirez
Copy link
Member

The diagnose report now checks on the presence of the appsignal.cjs file we require customers to initialize the AppSignal client when starting their apps.

Part of: #882

@@ -157,6 +156,9 @@ export class DiagnoseTool {
log_dir_path: {
path: logFilePath ? path.dirname(logFilePath) : ""
},
"appsignal.cjs": {
path: this.#config.clientFilePath || ""
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we could put the content of the file as well (see appsignal.log below) but we're not doing that because (as you said in this comment) we're going to try to get the config values in this file to be reported in the same way as the other config value sources, yes? Just checking to make sure I understood the comment and the plan.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep! As we can't control what and how users write their appsignal.cjs code, I think it's more effective doing it by loading the code and then checking the config object.

src/__tests__/config.test.ts Outdated Show resolved Hide resolved
src/__tests__/config.test.ts Outdated Show resolved Hide resolved
The diagnose report now checks on the presence of the `appsignal.cjs`
file we require customers to initialize the AppSignal client when
starting their apps.
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.

3 participants