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

feat: Make it a probot app ✨ #96

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

feat: Make it a probot app ✨ #96

wants to merge 22 commits into from

Conversation

mrchief
Copy link
Collaborator

@mrchief mrchief commented Feb 10, 2019

Fixes #85

expect(actualSuccessStatus).toMatchObject(successStatusBody);
});

test('POST /webhook should add a failure status to the PR if it doesn’t pass the users rules', async () => {
Copy link
Collaborator Author

@mrchief mrchief Feb 11, 2019

Choose a reason for hiding this comment

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

@ewolfe Can you please verify the accuracy of this test? Basically, ensure that the previous functionality is not lost?

@mrchief
Copy link
Collaborator Author

mrchief commented Feb 11, 2019

Should be good to go now. I copied existing logic into createFinalReport but I'd appreciate if you can verify those core pieces (lint + createFinalReport). If we can verify those 2 are doing what they are doing in current prod implementation, then this PR should be good to merge.

@ewolfe
Copy link
Owner

ewolfe commented Feb 11, 2019

@mrchief I probably won't get to spend anytime on this today. But I'll outline steps in case you want to get a jumpstart on testing it:

  1. Accept the GitHub org invite https://github.com/prlint
  2. Configure the permissions in the new staging app to match the existing app https://github.com/organizations/prlint/settings/apps/prlint-staging/permissions
  3. Deploy this branch to https://prlint-staging.now.sh/webhook (or anywhere, really)
  4. Install the prlint-staging app on a repo (a new sandbox repo in the prlint github org) and verify
  5. Merge this branch if all goes well

@mrchief
Copy link
Collaborator Author

mrchief commented Feb 11, 2019

@ewolfe Sounds like a plan. Will keep you posted.

@mrchief
Copy link
Collaborator Author

mrchief commented Feb 14, 2019

Tried creating a now deployment but things weren't going well. After banging my head in frustration, I found this: probot/probot.github.io#273

now doesn't let you create v1 deployments anymore. I created a v2 now.json

{
  "name": "prlint",
  "version": 2,
  "alias": "prlint-staging",
  "env": {
    "PRIVATE_KEY": "@prlint-private-key",
    "WEBHOOK_SECRET": "@prlint-webhook-secret",
    "NODE_ENV": "production"
  },
  "builds": [
    {
      "src": "*.js",
      "use": "@now/node"
    }
  ],
  "routes": [
    {
     "src": "/", "dest": "index.js"
    }
  ],
  "regions": [
    "all"
  ]
}

but it seems like we need that wrapper for things to work properly. Looks like Probot docs would need an update too. Will continue looking into this...

@ewolfe
Copy link
Owner

ewolfe commented Feb 14, 2019

Oh no! That sounds like it was probably really frustrating. I can help investigate over the weekend too.

@mrchief
Copy link
Collaborator Author

mrchief commented Feb 14, 2019

You bet! 3 hours of head-banging fun! I also tried the serverless-lambda but that resulted in Error: Cannot find module '/var/task/user/package' error. I'm going to give it a spin in a real AWS lambda (and not via now) to see if it makes any difference.

@mrchief
Copy link
Collaborator Author

mrchief commented Feb 25, 2019

I tried with AWS lambda and here's what I found:

  • we don't need to or rather shouldn't have handler.js because the handler.js logic differs between AWS and GCF.
  • using my own AWS handler.js and rest of the package, I was able to get the lambda to respond to install/ping etc events. It didn't mount to /webhook path even with the Probot env var so either I'm missing something or something else is amiss from Probot's side (could be a bug on them). It's not a big issue for me as /webhook is something we want to preserve for the now.sh deployments and not for internal/private deployments (although it'd be nice to have consistency).

So based on these, I think we can do one of these:

  • bundle now.sh, AWS, GCF specific handler.js files - not sure if this is a trend or strategy that we should own or let it be handled via userland (I prefer userland since every enterprise will have its own requirements which may not align with what we package or provide).
  • provide the probot app as a core package and move the now.sh specific implementation to its own repo. This'll be practically dogfooding and I think will serve a few different purposes:
    • let us validate core changes
    • will be a good example of how to take the core package and use it in a specific serverless deployment
    • add recipes for AWS/ GCF
    • futureproof us when now v3 comes out and screws everything up again 😆

Thoughts?

@mrchief
Copy link
Collaborator Author

mrchief commented Mar 5, 2019

@ewolfe Did you get a chance to look at it yet?

@mrchief mrchief changed the title feat: Make it a probot app ✨ [WIP: DO NOT MERGE YET] feat: Make it a probot app ✨ Mar 17, 2019
@mrchief
Copy link
Collaborator Author

mrchief commented May 17, 2019

@ewolfe Any update on this? I can merge this but I don't have access to the deployment service (now.sh I believe?).

If you can add me there, then I can test and get this version uploaded.

@ewolfe
Copy link
Owner

ewolfe commented May 20, 2019

@mrchief sorry for the delay. Yeah let's get this wrapped up! Can you signup with this URL https://zeit.co/teams/invite/qDB2l5r6 and then try deploying to https://prlint-staging.now.sh/ ?

@mrchief
Copy link
Collaborator Author

mrchief commented May 20, 2019

Great! I signed up but I wasn't able to install Now on prlint (complains that I'm not an owner which is weird). I'll try later via the cli.

I'll ping you on spectrum if needed. Don't want to bog down this PR thread with side chatter.

@ewolfe
Copy link
Owner

ewolfe commented May 20, 2019

@mrchief can you try installing it on https://github.com/prlint/prlint-staging ? I want to move this repo over to https://github.com/prlint/prlint as part of this whole update as well

@mrchief
Copy link
Collaborator Author

mrchief commented May 20, 2019

@mrchief can you try installing it on prlint/prlint-staging

I believe that is where I tried. But I'll check again tonight.

@mrchief
Copy link
Collaborator Author

mrchief commented May 21, 2019

Was able to sort thru issues. Did a test deployment at https://prlint-staging.mrchief.now.sh/ so I know integration etc. is now all setup correctly.

I'm going to start cleaning up this PR now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Self hosted version
2 participants