[Feature] Timeout + hooks (on-timedout, before-comment)

[fbartho]

Recently, we did a big folder move in preparation for having monorepo. -- Expand for Context

A consequence of this was that every file in the repo was tagged as having been modified. -- This then caused danger-plugin-eslint to execute on every file -- but due to a configuration error, this caused DangerJS to hang until terminated by Github Actions (20mins later) as each file individually did a lot of processing before throwing errors.

I think it would be very useful if I could configure danger with a hardcoded timeout so that it never takes long enough to fail to report.

Related to this, I had put code in a schedule block to truncate DangerJS output if it was too long as this was causing Github to fail to post the comment. -- Unfortunately, in the situation described above, this code actually executed before previous schedule blocks are complete meaning that it logged debug logs and implies that schedule-block execution is happening in parallel. -- I thought that schedule blocks are serially executed.

This misunderstanding means that my truncation code probably works only due to a timing fluke.

If I add a timeout feature, it would be useful to have a beforeCommentsPosted hook and an onTimedOut hook. The onTimedOut hook would be useful to add an additional message, while the beforeCommentsPosted hook would be useful to implement the truncation feature.

Any thoughts @orta or somebody else?

[fbartho]

Related: #1055

[orta]

This could be a general Dangerfile API which plugins could use:

import {danger} from "danger"

danger.hooks.beforeComments((violations: Voilations[])=> {

})

But a timeout in user-code evaluation isn't really the responsibility of Danger, though 20m is pretty wild. I'm not opposed to the idea of a timeout of 10m by default and then just posting something back to the PR.

[fbartho]

Yeah, that API looks good to me personally.

Do we also need/want APIs to "unregister hooks"? (Could just expose hooks.current.beforeComments[])

---

Just so we're clear: schedule does operate in parallel right?

[fbartho]

Is there a "flush current violations to CI" API?

-- If we build the beforeComments hook, I could build the timeout functionality in userspace if I have the ability to instruct danger not to wait for any remaining scheduled blocks.

Pseudo code:

const timeoutHandle = setTimeout(async () => {
    error("DangerJS Timed out. Partial Results may be attached");

    await danger.reportCurrentViolations();
    process.exit(1);
}, 10 * 60 * 1000);

danger.hooks.beforeComments(async (violations: Violations[]) => {
  clearTimeout(timeoutHandle);
  
  // Insert custom violation truncation/filtering code here
  return violations;
});

schedule(…);
schedule(…);
schedule(…);
schedule(…);

[orta]

Scheduled is only really useful in old builds of Peril, but generally I think it is parallel

There isn't a flush the violations, they're collected when the process has declared the node runtime is about to exit - so that's a good time to run any last hooks and allow tweaking violations etc

[fbartho]

Today-I-Learned: node-cleanup is a thing, also that sort of explains an aspect of the subprocess architecture that I had wondered about!

So based on my understanding, if I call process.exit(1) that will intrinsically report the current violations, so that simplifies things.

My thoughts as to how to proceed:

1. Add a new file in source/dsl

interface DangerRuntimeHooks {
  /** Register a Callback to be executed right before the Violations are published to CI/Checks/etc */
  beforeViolationsPublish: ((violations: Violations[]) => Promise<Violations[]>) => void;
}

(should this be called beforeComments beforeViolationsPublish -- something else?) 2. Expand DangerDSL to reference this type.

export class DangerDSL {
  public readonly github?: GitHubDSL
  public readonly bitbucket_server?: BitBucketServerDSL
  public readonly gitlab?: GitLabDSL
+
+  public readonly hooks: DangerRuntimeHooks;

  constructor(platformDSL: any, public readonly git: GitJSONDSL, public readonly utils: DangerUtilsDSL, name: string) {
    switch (name) {
      case "GitHub":
      case "Fake": // Testing only
        this.github = platformDSL
      case "BitBucketServer":
        this.bitbucket_server = platformDSL
      case "GitLab":
        this.gitlab = platformDSL
    }
  }
}

3. Add an entry to the nodeCleanup call that executes the beforeWhatever hook(s) that are scheduled.
4. Add to docs/changelog? (Should this just be an advanced feature?)

[orta]

Yep!

Maybe let's call it danger.hooks.addViolationsProcessor((violations: Violations[]) => Promise<Violations[]>) so that many plugins/dangerfiles could use it and it can pass through all of them in order, I dunno what folks want to do but I'd like to give the widest scope with the smallest API:

danger.hooks.addViolationsProcessor((v) => v.filter(v.line === 1)) // it'd run this  first
danger.hooks.addViolationsProcessor((v) => [...v.filter(v.filename !== "README.md"), customViolation]) // then the results would go into this 

And it would not show results for line one anywhere and skip all the readme and add a new violation.

Calling exit would finish the danger file evaluation completely though (because danger is multi-process but it will kick it back to the reporting process ) so a plugin shouldn't do it ideally, unless something is pretty critical

[fbartho]

Re: Name: danger.hooks.addViolationsProcessor((violations: Violations[]) => Promise<Violations[]>) ✅ Agreed!

---

Re: process.exit(1) I would only call that in my userspace code to warn that stuff has timed out (knowing that there would be some data-loss).

I'm currently not planning on adding a built-in timeout to DangerJS unless you think it would be particularly valuable? -- I could provide this as a recipe in documentation or something to avoid exposing further API?

---

@orta Is there anything else I should capture before executing on this? -- I'll probably get around to it later this week, but it's Monday so… best laid plans and all that jazz.

[orta]

I can't think of any gotchas, perhaps there's a space for two APIs instead of one:

// called anytime _any_ violation is created
danger.hooks.addViolationsProcessor((violations: Violations) => Promise<Violations[]>)

// called at the end 
danger.hooks.addViolationsFinalizedProcessor((violations: Violations[]) => Promise<Violations[]>)

[fbartho]

@orta -- I admit that I'm confused about the surface area / distinction between the two.

It sounds like the callbacks passed to addViolationsFinalizedProcessor are executed sequentially from nodeCleanup -- but I don't understand when addViolationsProcessor's callbacks are executed.

Based on the comment (called anytime _any_ violation is created), it feels like addViolationsProcessor is intended to executed immediately when somebody calls error(…) -- but then the signature doesn't match that use-case. (Does this make sense?)

Recapping my pseudo-code from earlier, which method would I use for each case? -- What cases were you thinking of?

const timeoutHandle = setTimeout(async () => {
    error("DangerJS Timed out. Partial Results may be attached");
    process.exit(1);
}, 10 * 60 * 1000);

// Should this be addViolationsProcessor or should this be addViolationsFinalizedProcessor?
danger.hooks.addViolationsFinalizedProcessor(async (violations: Violations[]) => {
  clearTimeout(timeoutHandle); // Would this even work since danger is multi-process?
  return violations
});

// Should this be addViolationsProcessor or should this be addViolationsFinalizedProcessor?
danger.hooks.addViolationsFinalizedProcessor(async (violations: Violations[]) => {
  if (countNumberOfCharactersInViolations(violations) > MAX_VIOLATIONS_COMMENT_SIZE) {
     return truncateViolationsToLength(MAX_VIOLATIONS_COMMENT_SIZE);
  }
  return violations
});

schedule(…);
schedule(…);
schedule(…);
schedule(…);

[orta]

danger.hooks.addViolationsProcessor happens every time anyone creates a violition, letting you amend, add an extra or remove it because it comes in as a singular and leaves as an array

danger.hooks.addViolationsFinalizedProcessor happens as the process is closing, with all of them

Truncating should be in core https://github.com/danger/danger-js/issues/919 - but your sample is a good case for the split, you want a timeout which is reset when a violation has been submitted so you use danger.hooks.addViolationsProcessor.

[eppisapiafsl]

Is there any update on this feature request?

I face a similar issue, my GitHub action end successfully, but the "Danger JS" gets stuck without posting the comment or result of yarn danger ci