Project hooks and github status hooks on cypress v5 #148

jhicken · 2020-09-28T06:36:49Z

So this relates to...
#144
#14
#143

And has implications for #120

This introduces a new set of forms on the project edit view that allow you to setup a few types of hooks.

Webhooks: These are pretty straight forward. Every time one of the event hooks are called, json representations of the run and instance involved in the event plus some extra stats data will be sent in a POST call from sorry-cypress to url that is defined. You can also provide headers that will be passed in the post call.

Github Status Hooks: Every time a run starts, finishes or an instance finishes. The provided github repository will receive a status update with the current results and a link to the run. Currently a github personal access token with status access on the repo is required.

github status

This is currently for v5 but I have a non v5 version of the branch as well if we need it.

agoldis · 2020-09-29T04:22:28Z

That's 💥 will get it out soon!

* origin/next: Use eslint Update dependencies, use @babel/node

agoldis

This is super super great 🚀💥⚡️. Thanks a lot for your great ideas and work!

I have changed the base for that PR to next
I have pushed few changes to next to put some focus on dev experience and code quality - eslint, typescript errors and prettier. Now there are more people working on that I want to keep the project clean and avoid broken windows.

As for the code / implementation:

Let's try to keep api and director typed, I didn't do a great job with types on dashboard and have been suffering. Please tell me is that's too much effort, I will try to help
Did you consider a pub/sub for communicating events in director? I am concerned by putting too much logic into routing functions.
I noticed few opportunities to use optional chaining.

Also, see inline comments

packages/director/package.json

packages/api/src/datasources/projects.ts

agoldis · 2020-09-30T08:20:54Z

packages/director/src/app.ts

  }

  console.log(`>> Machine is joining a run`, { ciBuildId });

-  const response = await app.get('executionDriver').createRun(req.body);
+  const response = await executionDriver.createRun(req.body);
+  const runWithSpecs = await executionDriver.getRunWithSpecs(response.runId);


What do you think about a pub/sub here?

pub.emit(hookEvents.RUN_START, { runId: response.runId });

The only problem is getting the execution driver, please let me know if you think that is a good option.

This is probably personal opinion. In my early days of programming I delt with the event emmiter pattern a lot and grew a major distaste for it. Particularly when stuff goes wrong and your emitter is hooked up to multiple subscribers. Tracking down subscriber order is a massive pain. Also just putting break points and trying to debug into a subscriber is kinda succy. Since event emitters push things to the back of the call stack.

If your not using the built in event system its not as big of a deal. Cause you can follow the code flow a little better. But I generally shy away from it cause I find it more understandable to track direct function calls.

That said if you would like me to change it I still can.

I agree that it can go left when abused - there are pros and cons. I did see emitters working nicely when controlled and used strictly. I also suffered a lot while working on systems that abused the pattern and lost control of emitters / listeners.

Bottom line, if you dislike emitters so much, let's try to find a different solution. What I am looking for is to separate the functionality that deals with director primary responsibility (manage the tests) from hooks functionality.

Simply extracting all hooks functionality from app.ts into a separate module would also work for me.

agoldis · 2020-09-30T08:23:24Z

packages/director/src/app.ts

+    const isRunStillRunning = run.specs.reduce(
+      (
+        wasRunning: boolean,
+        currentSpec: {
+          claimed: boolean;
+          results: any;
+        },
+        index: number
+      ) => {
+        return (
+          !currentSpec.claimed || !run.specsFull[index].results || wasRunning
+        );
+      },
+      false
+    );


Should it be a distinct method?

What alternative are you thinking of?

I separated it into a function for readability. If thats what you are asking about.

packages/director/src/app.ts

packages/director/src/execution/mongo/runs/run.model.ts

packages/director/src/lib/hooksReporter.ts

jhicken · 2020-09-30T19:05:19Z

I replied to everything inline.

Let's try to keep api and director typed, I didn't do a great job with types on dashboard and have been suffering. Please tell me is that's too much effort, I will try to help

Thanks! Yeah help on that would be super appreciated. Sorry if im a pain on that. I'm not a big typescript user. Im happy to make whatever changes you think we need to. So if there are places you want me to change types just call them out. Also there seems to be a lot of ways and places to define said types. Would you like them in a single file? Do you want them in the source file that is most closely related to them. Do you want them inline on functions themselfs. Do you want them in a file separate file with the same name as the file that your working on. Do we wanna be sharing them across <packages>? Sorry part of why its hard for me is that I dont think we have it nailed down when and where types are created and stored.

Guide me please.

agoldis · 2020-09-30T19:42:23Z

So if there are places you want me to change types just call them out. Also there seems to be a lot of ways and places to define said types.

Would you like them in a single file? No
Do you want them in the source file that is most closely related to them. See below
Do you want them inline on functions themselves. See below
Do you want them in a file separate file with the same name as the file that your working on. No
Do we wanna be sharing them across ? Ideally yes, but we don't have a good solution for that now.

I am not opinionated about that, what is important is consistency. Suggesting to mimic the current director structure.

For trivial types inline function would work (example)

For more complex types, let's have a types file that is scoped a specific domain, e.g. s3 screenshot driver has its own types example

For cross-functional shared types let's use root "types" directory.

For generated types (from GraphQL) - generated/... directory

I am marking the specific functions missing types...

agoldis · 2020-09-30T19:43:43Z

packages/api/src/datasources/projects.ts

 import { getMongoDB, init } from '@src/lib/mongo';
 import { DataSource } from 'apollo-datasource';
+import uuid from 'uuid/v4';


Suggested change

import uuid from 'uuid/v4';

import uuid from 'uuid/v4';

import { Project } from '@src/duplicatedFromDirector/project.types';

agoldis · 2020-09-30T19:44:02Z

packages/api/src/datasources/projects.ts

@@ -19,6 +21,56 @@ const getSortByAggregation = (direction = 'DESC') => ({
  },
 });

+const addHookIdsToProjectHooks = (project) => {


Suggested change

const addHookIdsToProjectHooks = (project) => {

const addHookIdsToProjectHooks = (project: Project) => {

agoldis · 2020-09-30T19:44:18Z

packages/api/src/datasources/projects.ts

+  return project;
+};
+
+const removeUnusedHookDataFromProject = (project) => {


Suggested change

const removeUnusedHookDataFromProject = (project) => {

const removeUnusedHookDataFromProject = (project: Project) => {

agoldis · 2020-09-30T19:44:41Z

packages/api/src/datasources/projects.ts

@@ -27,29 +79,37 @@ export class ProjectsAPI extends DataSource {
  async getProjectById(id: string) {
    const result = getMongoDB()
      .collection('projects')
-      .aggregate([{
+      .aggregate([


Suggested change

.aggregate([

.aggregate<Project>([

agoldis · 2020-09-30T19:45:00Z

packages/api/src/duplicatedFromDirector/project.types.ts

+  hookId: string;
+  url: string;
+  headers: string;
+  hookEvents: [string];


Suggested change

hookEvents: [string];

hookEvents: string[];

agoldis · 2020-09-30T19:45:11Z

packages/api/src/duplicatedFromDirector/project.types.ts

+export interface Project {
+  projectId: string;
+  createdAt: string;
+  hooks?: [Hook];


Suggested change

hooks?: [Hook];

hooks?: Hook[];

agoldis · 2020-09-30T19:45:40Z

packages/dashboard/src/duplicatedFromDirector/project.types.ts

+  hookId: string;
+  url: string;
+  headers: string;
+  hookEvents: [string];


Suggested change

hookEvents: [string];

hookEvents: string[];

agoldis · 2020-09-30T19:45:50Z

packages/dashboard/src/duplicatedFromDirector/project.types.ts

+export interface Project {
+  projectId: string;
+  createdAt: string;
+  hooks?: [Hook];


Suggested change

hooks?: [Hook];

hooks?: Hook[];

* origin/next: Fine tune eslint and add tsc watch

jhicken · 2020-10-02T22:14:41Z

Sweet. Would you like me to make these extra changes in another PR?

agoldis · 2020-10-02T22:28:07Z

Sweet. Would you like me to make these extra changes in another PR?

If you already done some refactoring - sure, otherwise don't waste your time :)

jhicken added 2 commits September 28, 2020 00:09

Adding webhooks and github status hooks.

6524912

Fix nav updates for project edit view.

d683051

jhicken mentioned this pull request Sep 28, 2020

Make the project cypress 5 compatible #137

Merged

agoldis changed the base branch from master to next September 30, 2020 06:49

Merge remote-tracking branch 'origin/next' into pr/jhicken/148

c29fb70

* origin/next: Use eslint Update dependencies, use @babel/node