Feature/455 (Github pull integration #455) #524
Conversation
…mess..." This reverts commit 3844aef.
…eeded (?)\n & Added some docs/comments
…, model & controller
…e with DB disconnecting before saving...
|
Could you give a description of how everything works? |
server/workers/github.js
Outdated
| function (projects) { | ||
| projects.forEach( | ||
| function (project) { | ||
| console.error("PROJECT: ", project.fullRepoPath); |
server/workers/github.js
Outdated
| function (commitData) { | ||
| var newCommit = new Commit(commitData); | ||
|
|
||
| // super hacky, definitely need to abstract this before github's api changes and this goes boom!! |
There was a problem hiding this comment.
Is there any way to record the author or prevent loading the same commit twice?
There was a problem hiding this comment.
The author field of the Commit model is currently populated & saved. As for preventing the loading of the same commit, do you mean when scraping Github, or actually running a save to the DB?
There was a problem hiding this comment.
For saving to the db. Like if it has the same hash as one we already saved, update the data instead of adding a second copy. Although, it might also be good to save the last updated date and ignore everything before it in the github query.
server/workers/github.js
Outdated
| function (projects) { | ||
| projects.forEach( | ||
| function (project) { | ||
| console.error("PROJECT: ", project.fullRepoPath); |
There was a problem hiding this comment.
What happens if the project url is invalid?
server/workers/github.js
Outdated
| ).then( | ||
| function () { | ||
| // @TODO FIX ASYNC ISSUES SO DISCONNECT HAPPENS AFTER ALL SAVES HAVE COMPLETED. | ||
| // Spent like 4 hours trying to debug this, but javascript is just too magical and asynchronous for me =( |
There was a problem hiding this comment.
Comments like these are best for the issue or PR, so that people can discuss them. What have you tried?
|
How often would this be run? Is there any way for an admin to run a batch? |
server/workers/github.js
Outdated
| var octoclient = octo.client(token); | ||
|
|
||
| Project.find().exec().then( | ||
| function (projects) { |
There was a problem hiding this comment.
It would be better if this was modular and could be run on only a specific set of projects
There was a problem hiding this comment.
Yeah, good point. Right now it's running on all projects, which definitely wouldn't be ideal behavior in the production env.
Also, thanks for taking the time to look over my PR. I'll start working on these changes as soon as I can.
|
If you run "node ./server/workers/github.js" it will attempt to populate commits for all existing projects in the database. However, because the DB is not disconnecting, it will hang instead of closing after finishing. But you can browse the commits collection and view the data. Also, the commit api endpoints for users should be working. However, the project endpoint isn't working yet because the query is wrong. |
|
@zmike808 have a chance to make any of the changes that Kiana has suggested? |
…eamline. Currently only a P.O.C. Will abstract soon.
|
Made a ton of changes. Much cleaner code & everything is currently working. Please check it out and let me know what you guys think so far, @kmcnellis @agundy. ALSO: Make sure to run npm install, since I switched to a different github API wrapper/library |
|
Status Update: almost done. Just need to integrate it with our endpoints. Know exactly what I have to do, just need to figure out how to do it in javascript & ExpressJS. Which sounds like it should be easy...but javascript. Anyways, I'm planning on making a new branch and PR once I have everything done. Just to make things cleaner. |
|
@zmike808 Sounds good! Let us know when we can look at it or if you need anything |
|
I think its actually better to just leave it on this branch / pr, btw. It saves history for anyone looking back to see how it was done. It'll be squashed before merging anyways. |
|
Hi, just wanted to drop a quick update. I know grades are due soon, and we were suppose to have everything in by today. I've been working on this last bit and it's been taking much longer than I thought. I am going to try to give it another hour or two, and if I can't get the api integration working, I'll just leave it as a function/on demand worker that can be run by nodejs cli. |
…ed functions to get commits for a project with date filtering
|
I couldn't end up getting the endpoints to work. I ended up just adding a function into the worker which will grab commits for alll projects. I also added functions to the worker to get commits for a project with date filtering, i.e. before x, or after y. |
Implemented feature #455. Only issue left is solving an async issue with the DB disconnecting before the commits finish saving. Besides that, everything is working. So, I'd like to get my code reviewed and hopefully merged in.