added support for task lists #460

tunix · 2016-10-10T22:20:12Z

Hi Everyone,

I've added support for task lists in pull requests. I haven't written the tests yet, but I'll. But before that, can somebody help me with the following problem?

During development, when I make a request for failing the commit, I'm getting 404 from GitHub API. But when I try the same request manually on the console, I'm getting 200 OK. Below is my request details. Can someone please help me with that?

$ cat <<EOF|http POST https://api.github.com/repos/tunix/zappr/statuses/b057484e4d5241751a4efd979b2f7b9aec916f3b "User-Agent:Zappr (+http://xxx.com:9999)" "Authorization:token xxx"
{"state":"failure","context":"zappr/pr/tasks","description":"tunix/zappr#4: Failed the PR due to open tasks."}
EOF
HTTP/1.1 201 Created

I'm getting below log when I try it through zappr:

zappr:github:error 2016-10-10T21:52:57Z 404 POST /repos/tunix/zappr/statuses/b057484e4d5241751a4efd979b2f7b9aec916f3b +464ms { message: 'Not Found', documentation_url: 'https://developer.github.com/v3' }

I'm also getting following error when I run tests against the server:

  235 passing (6s)
  1 failing

  1) API "before each" hook for "should work with token and no session":
     TypeError: Cannot read property 'map' of undefined
      at test/MountebankClient.js:75:44
      at next (native)
      at step (test/MountebankClient.js:29:191)
      at test/MountebankClient.js:29:361
      at process._tickDomainCallback (internal/process/next_tick.js:129:7)

tunix · 2016-10-11T06:31:46Z

@prayerslayer I solved the commit message problem with this one. I forked this repo as tunix/zappr and cloned it onto my computer. Then I followed the usual things:

npm install
npm run build
HOST_ADDR=http://xxx:9999 GITHUB_CLIENT_ID=xxx GITHUB_CLIENT_SECRET=xxx LOG_LEVEL=debug DEBUG=zappr*,github* npm start

Then through the UI, I first verify that zappr is able to read zappr config from the repo, then enable new hook for PR Tasks, check that it started protecting master branch and created the hook at tunix/zappr repo. All these steps are just fine.

I also added debug lines into server/handler/HookHandler.js to check what parameters are being passed to the GitHub API and they seem fine to me. I also checked whether the scope of the oAuth tokens are correct and I couldn't see a problem there as well.

prayerslayer · 2016-10-11T08:06:18Z

Hmm that sounds correct. Can you verify it works manually using the same token that Zappr uses?

Regarding the failing test, what's the output of node -v && npm -v? It works on Travis and Travis builds with Node 5.7. Maybe that's a problem?

tunix · 2016-10-11T08:11:26Z

@prayerslayer I confirm it works with the token Zappr uses. Maybe the issue is related to my dev environment since I'm using SSH tunnels for the sake of easy development instead of deploying it to some server. I was using node 6.7.0 :) I'll try with 5.7, thanks!

prayerslayer · 2016-10-11T08:20:22Z

Uh, I don't know enough about SSH tunnels to comment on that, I'm afraid... We use localtunnel to make our dev environment on localhost available to Github:

npm i -g localtunnel
# just in case
unalias lt
# expose localhost:3000
lt -s myzapprhost -p 3000
# run zappr
export HOST_ADDR=https://myzapprhost.localtunnel.me
npm run all

Don't forget to change your OAuth app settings accordingly.

prayerslayer · 2016-10-13T13:14:53Z

Hey, I wanted to follow up on this. Did you try my suggestion? Did you get it to run otherwise?

tunix · 2016-10-13T13:50:05Z

Hi @prayerslayer,

Sorry I didn't have time to look at it. I'll try and let you know about it asap.

prayerslayer · 2016-10-13T13:57:29Z

Take your time, I just wanted to know the status :)

tunix · 2016-10-14T20:41:20Z

@prayerslayer it worked :) thanks! i'll add the tests now.

tunix · 2016-10-17T12:48:54Z

Hi All,

This PR is now ready for a review. :) Thanks!

prayerslayer · 2016-10-17T14:06:45Z

.node-version

@@ -0,0 +1 @@
+5.7.1


Where does this come from? Which tools is supposed to pick it up?

This is for nodenv (for managing node versions). It automatically picks up the correct node version once you enter into the directory.

Ah, ok. Personally I use nvm for that purpose, so let's also add a .nvmrc 😁

Will you add it because I've never used nvm before?

I would need write access to your fork, no? It's the same content and the filename is .nvmrc. https://github.com/creationix/nvm#nvmrc

prayerslayer · 2016-10-17T14:09:27Z

server/checks/PullRequestTasks.js

+const debug = logger(CHECK_TYPE)
+const createStatePayload = getPayloadFn(CONTEXT)
+
+export function hasOpenTasks(pr) {


Maybe only pass the PR body as parameter? Everything else is just for logging which you can do outside of the function.

When printing debug messages, I'm doing it inside the loop and to seperate those lines from other PR's I need to include repoName and prNo so that prevents me from just including pr body.

Hm, got it. I really don't like passing a full-fledged PR object into a function that's supposed to work on a string. Can we add repoName and prNo as optional parameters?

function hasOpenTasks(prBody, repoName = '', prNumber = '') { ... }

prayerslayer · 2016-10-17T14:15:53Z

server/checks/PullRequestTasks.js

+        let msg;
+
+        if (hasOpenTasks(pull_request)) {
+          msg = `${fullName}#${number}: Failed the PR due to open tasks.`;


Please don't include repo name and PR number in status message, we also don't do it in other checks.

As for messages, what about "PR has ${openTasks} open tasks." in case of failure?

I'll change the code accordingly.

prayerslayer · 2016-10-17T14:20:53Z

server/checks/PullRequestTasks.js

+    let status;
+
+    try {
+      // TODO: configuration settings


What about this?

In fact I couldn't think of any settings at all for this feature. It's only about enabling/disabling it at this moment. But I placed a TODO just in case someone wants to add some configuration flags in the future.

Okay, then let's just remove that TODO. Looks like we forgot something.

tunix · 2016-10-17T14:46:19Z

@prayerslayer Added requested changes.

prayerslayer · 2016-10-17T14:48:01Z

server/checks/PullRequestTasks.js

+
+    try {
+      if (pull_request.state === 'open' && ['opened', 'edited', 'reopened'].indexOf(action) !== -1) {
+        let openTaskCount = countOpenTasks(pull_request.body, repoName, number);


Ah, sorry if my suggestion was misleading. Putting the fullName inside there is a good idea as it's otherwise hard to distinguish different repos with the same name.

tunix · 2016-10-17T14:52:43Z

Done 😄

prayerslayer · 2016-10-17T15:01:34Z

Looks good! We now need to write a concise info text for the frontend (see RepositoryCheck.jsx and some documentation). Then it's done!

tunix · 2016-10-17T15:02:44Z

@prayerslayer do you expect anything else from me? is it something i should do?

prayerslayer · 2016-10-17T15:04:14Z

Hm not really, I can write the docs myself :) I'll wait for the build to finish and see to get it merged.

prayerslayer · 2016-10-17T15:11:04Z

👍

sebastianpoeplau · 2016-10-17T15:28:42Z

👍

prayerslayer · 2016-10-17T15:30:45Z

Thanks for the effort @tunix!

* #440 docs and db migration * #440 remove dot

tunix force-pushed the master branch 3 times, most recently from 125b5ad to 98e36e8 Compare October 17, 2016 12:44

prayerslayer reviewed Oct 17, 2016

View reviewed changes

tunix force-pushed the master branch from 98e36e8 to 36286e0 Compare October 17, 2016 14:45

prayerslayer reviewed Oct 17, 2016

View reviewed changes

zalando#440 feat: added support for task lists in PR

5b14bd8

tunix force-pushed the master branch from 36286e0 to 5b14bd8 Compare October 17, 2016 14:52

zalando#440 feat: added .nvmrc

4bd4064

prayerslayer merged commit 778f73a into zalando:master Oct 17, 2016

prayerslayer mentioned this pull request Oct 17, 2016

Docs and db migration for #460 #463

Merged

prayerslayer added a commit that referenced this pull request Oct 18, 2016

Docs and db migration for #460 (#463)

681f4db

* #440 docs and db migration * #440 remove dot

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

added support for task lists #460

added support for task lists #460

tunix commented Oct 10, 2016

tunix commented Oct 11, 2016

prayerslayer commented Oct 11, 2016

tunix commented Oct 11, 2016

prayerslayer commented Oct 11, 2016

prayerslayer commented Oct 13, 2016

tunix commented Oct 13, 2016

prayerslayer commented Oct 13, 2016

tunix commented Oct 14, 2016

tunix commented Oct 17, 2016 •

edited

prayerslayer Oct 17, 2016

tunix Oct 17, 2016 •

edited

prayerslayer Oct 17, 2016

tunix Oct 17, 2016 •

edited

prayerslayer Oct 17, 2016

prayerslayer Oct 17, 2016

tunix Oct 17, 2016

prayerslayer Oct 17, 2016

prayerslayer Oct 17, 2016

tunix Oct 17, 2016

prayerslayer Oct 17, 2016

tunix Oct 17, 2016

prayerslayer Oct 17, 2016

tunix commented Oct 17, 2016

prayerslayer Oct 17, 2016

tunix commented Oct 17, 2016

prayerslayer commented Oct 17, 2016 •

edited

tunix commented Oct 17, 2016

prayerslayer commented Oct 17, 2016

prayerslayer commented Oct 17, 2016

sebastianpoeplau commented Oct 17, 2016

prayerslayer commented Oct 17, 2016

		@@ -0,0 +1 @@
		5.7.1

added support for task lists #460

added support for task lists #460

Conversation

tunix commented Oct 10, 2016

tunix commented Oct 11, 2016

prayerslayer commented Oct 11, 2016

tunix commented Oct 11, 2016

prayerslayer commented Oct 11, 2016

prayerslayer commented Oct 13, 2016

tunix commented Oct 13, 2016

prayerslayer commented Oct 13, 2016

tunix commented Oct 14, 2016

tunix commented Oct 17, 2016 • edited

Choose a reason for hiding this comment

tunix Oct 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunix Oct 17, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tunix commented Oct 17, 2016

Choose a reason for hiding this comment

tunix commented Oct 17, 2016

prayerslayer commented Oct 17, 2016 • edited

tunix commented Oct 17, 2016

prayerslayer commented Oct 17, 2016

prayerslayer commented Oct 17, 2016

sebastianpoeplau commented Oct 17, 2016

prayerslayer commented Oct 17, 2016

tunix commented Oct 17, 2016 •

edited

tunix Oct 17, 2016 •

edited

tunix Oct 17, 2016 •

edited

prayerslayer commented Oct 17, 2016 •

edited