Move CI from Travis to GitHub Actions #234

dividedmind · 2023-07-10T15:05:21Z

Note for deploy to work NPM_TOKEN needs to be provided as a secret.

lachrist

Here are my three remarks:

consider that npm run coverage does perform integration tests.
I would move log changes to another PR.
fswin seems heavy and polute the pure path component.

components/hook-query/node/pg.test.mjs

lachrist · 2023-07-18T12:13:10Z

.github/workflows/ci.yml

+        if: matrix.os == 'ubuntu'
+      - run: npm run coverage
+        if: matrix.node-version == 18 && matrix.os == 'ubuntu'
+      - run: npm run test


This should not be conditional because nom run coverage does not perform integration tests.

Good point, I missed that. I've split test into separate scripts in 614b0f7.

lachrist · 2023-07-18T12:15:56Z

components/command/node/jest-config.mjs

@@ -42,7 +42,9 @@ const loadPackageJsonAsync = async (directory) => {
      return null;
    } else {
      logError("Could not load package.json from %j >> %O", directory, error);
-      throw new ExternalAppmapError("Could not load package.json");
+      throw new ExternalAppmapError(
+        format("Could not load package.json (%O)", [error]),


In the rest of the codebase, I use log for detailed dynamic information and error message for a short static description. I'm not saying that this worse but it is not consistent with the other (many) location where this pattern is present.

The problem is, turning logging on is another step which can cause friction and it provides a lot of potentially unnecessary information. I don't want to have to rerun CI and delve through all the log messages to figure out what the actual single error was. Current laconic error messages give the user zero indication of what actually went wrong. We don't need (and we shouldn't) give the user a stacktrace by default (although we actually do!), but I believe we should (and need to) at least tell the user more specifically want went wrong instead of a enigmatic generic message that leaves them puzzled and confused.

components/util/default/error.mjs

lachrist · 2023-07-18T12:28:46Z

package.json

@@ -44,6 +44,7 @@
    "ajv-error-tree": "0.0.5",
    "astring": "^1.8.4",
    "chalk": "^5.0.1",
+    "fswin": "^3.23.311",


Couple of questions/remarks:

Could you elaborate on when/how the short path shown its ugly head?

Do we have to use this third-party package? I'm nervous at the idea of introducing a c++ depencency to solve what seems to be a minor problem.

The path component should remain pure. We should either provide the information necessary to toDirectoryPath solve short paths in a pure manner. Or solve them at the callcite.

Couple of questions/remarks:

* Could you elaborate on when/how the short path shown its ugly head?

There are a couple of places where the code does sanity checks on code paths (for example here) and because it arrives at the path through different methods it can cause one to be short and the other to be long (see for example this error).

* Do we have to use this third-party package? I'm nervous at the idea of introducing a c++ depencency to solve what seems to be a minor problem. * The path component should remain pure. We should either provide the information necessary to `toDirectoryPath` solve short paths in a pure manner. Or solve them at the callcite.

We have to call the system API; this is the only way to resolve a short path from a long one and vice-versa. It's not possible to do it in a pure way since the path mapping is stored in the filesystem and the only way to access it is through this API. And fswin is just a very thin wrapper on the relevant APIs. The alternative would be to write it ourselves :)

lachrist · 2023-07-18T12:33:23Z

components/error/default/index.mjs

@@ -46,8 +46,14 @@ export class InternalAppmapError extends AppmapError {
 }

 export class ExternalAppmapError extends AppmapError {
-  constructor(message) {
+  constructor(message, from = null) {


Same remark as for your previous dynamic error message. I'm not against improving the current logging mechanism (detailed dynamic log + short static error message) but I don't see the point in changing it in two places where there are like 100+ locations where it is used. In my opinion this should be discussed and changed in a separate PR so that we can group all these change together and retain some consistency.

Like I said before, I think short error message needs to contain some specific information to be actually useful. Otherwise it's just annoying and useless. But I agree that it's better to strategically improve all the sites that can benefit from it. I just put these in as I needed to solve the problems I encountered.

OTOH, I don't want this to become another rabbit hole we spend too much time on while we have other issues to deal with. Now I think maybe it's okay to keep those tactical changes and add an issue to address this problem more systematically. If we find the time and impetus to do it while doing some other work, even better, but perhaps we shouldn't take out those tactical changes just for the sake of purity; if I encountered these, perhaps the users will (do) too.

dividedmind · 2023-07-19T15:49:20Z

Can you take another look? Obviously please don't merge as-is, I need to rebase the fixups, I'll take care of it before merging.

Here are my three remarks:

* consider that `npm run coverage` does perform integration tests.

Fixed.

* I would move log changes to another PR.

Thinking about it some more, I think we should keep the tactical changes and open an issue for a more strategic treatment on this. We need to keep moving forward :)

* `fswin` seems heavy and polute the pure path component.

It's the opposite of heavy, it's a dependency-less thin wrapper on the relevant OS APIs. Perhaps it does pollute the path component a bit, but I don't see much of a way around it without needlessly complicating the code. Every abstraction is leaky in the end :)

brikelly · 2023-08-02T14:27:45Z

@lachrist can you please respond to @dividedmind's comments?

It seems sometimes on Windows the error code returned when a file cannot be found is UNKNOWN.

Sometimes paths on Windows are represented by short paths such as C:\PROGRA~1. This leads to issues if we try to compare paths and one of them is equivalent to the other, but shortened. To solve this issue, try to resolve the long path when resolving directory paths (which is the only place I've seen this problem occur so far). It specifically lead to failures on CI where TEMP directory is a short path.

Timeout for subcommands used to be 1 second, which lead to commands randomly timing out in tests and likely also random crashes in user deployments (which could have been very mysterious due to previously laconic error messages).

Official support ended more than two months ago.

This is driven by `npm audit`. Note some potential vulnerabilities signalized by it cannot currently be dealt with, but they apply only to packages used in development so are unlikely to affect us.

We've seen intermittent errors in our test runs in CI similar to this one: TypeError: Cannot read properties of undefined (reading '0') at parseURL (@appland/appmap-agent-js/dist/bundles/server.mjs:4078:8) at Object.request (@appland/appmap-agent-js/dist/bundles/server.mjs:4207:42) at digestPayload (@appland/appmap-agent-js/dist/bundles/server.mjs:4264:25) ... It's not clear why this happens. Perhaps if the error message printed the problematic payload it'll be clearer what the problem is.

appland-release · 2023-08-04T17:05:50Z

🎉 This PR is included in version 13.9.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

appland-release · 2023-08-04T17:30:51Z

🎉 This PR is included in version 14.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

dividedmind requested a review from lachrist July 10, 2023 15:05

dividedmind self-assigned this Jul 10, 2023

dividedmind force-pushed the ci/move-to-github-actions branch 2 times, most recently from 887cbea to 7122115 Compare July 10, 2023 16:32

dividedmind mentioned this pull request Jul 11, 2023

Show more information when failing to digest a payload #232

Closed

lachrist suggested changes Jul 18, 2023

View reviewed changes

dividedmind force-pushed the ci/move-to-github-actions branch 2 times, most recently from 92b7d09 to 614b0f7 Compare July 18, 2023 13:58

dividedmind requested a review from lachrist July 19, 2023 15:46

dividedmind force-pushed the ci/move-to-github-actions branch from ae0954b to 14f61ff Compare August 3, 2023 12:46

lachrist approved these changes Aug 4, 2023

View reviewed changes

dividedmind added 12 commits August 4, 2023 17:18

ci: Remove travis configuration

3fb052b

ci: Add Github Actions workflow

a8ad284

Show more information on error reading package.json

aa70c69

fix: Tolerate UNKNOWN errors on Windows

4f178f6

It seems sometimes on Windows the error code returned when a file cannot be found is UNKNOWN.

Show more information in error message when failing to spawn

098d413

fix: Wait a bit longer while spawning subcommands

9d6ff47

Timeout for subcommands used to be 1 second, which lead to commands randomly timing out in tests and likely also random crashes in user deployments (which could have been very mysterious due to previously laconic error messages).

BREAKING CHANGE: Remove support for Node 14

e3212b6

Official support ended more than two months ago.

Update some dependencies to remove potential vulnerabilities

c23b5cd

This is driven by `npm audit`. Note some potential vulnerabilities signalized by it cannot currently be dealt with, but they apply only to packages used in development so are unlikely to affect us.

Fix a small but highly visible typo in error messages

d89983a

ci: Use correct tokens with semantic-release

27114f3

dividedmind force-pushed the ci/move-to-github-actions branch from 14f61ff to 27114f3 Compare August 4, 2023 16:18

dividedmind merged commit 27114f3 into main Aug 4, 2023
8 checks passed

dividedmind deleted the ci/move-to-github-actions branch August 4, 2023 16:25

dividedmind restored the ci/move-to-github-actions branch August 4, 2023 16:45

appland-release added the released label Aug 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move CI from Travis to GitHub Actions #234

Move CI from Travis to GitHub Actions #234

dividedmind commented Jul 10, 2023

lachrist left a comment

lachrist Jul 18, 2023

dividedmind Jul 18, 2023

lachrist Jul 18, 2023

dividedmind Jul 19, 2023

lachrist Jul 18, 2023

dividedmind Jul 19, 2023

lachrist Jul 18, 2023

dividedmind Jul 19, 2023 •

edited

Loading

dividedmind commented Jul 19, 2023

brikelly commented Aug 2, 2023

appland-release commented Aug 4, 2023

appland-release commented Aug 4, 2023

Move CI from Travis to GitHub Actions #234

Move CI from Travis to GitHub Actions #234

Conversation

dividedmind commented Jul 10, 2023

lachrist left a comment

Choose a reason for hiding this comment

lachrist Jul 18, 2023

Choose a reason for hiding this comment

dividedmind Jul 18, 2023

Choose a reason for hiding this comment

lachrist Jul 18, 2023

Choose a reason for hiding this comment

dividedmind Jul 19, 2023

Choose a reason for hiding this comment

lachrist Jul 18, 2023

Choose a reason for hiding this comment

dividedmind Jul 19, 2023

Choose a reason for hiding this comment

lachrist Jul 18, 2023

Choose a reason for hiding this comment

dividedmind Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

dividedmind commented Jul 19, 2023

brikelly commented Aug 2, 2023

appland-release commented Aug 4, 2023

appland-release commented Aug 4, 2023

dividedmind Jul 19, 2023 •

edited

Loading