Conversation
|
Don't merge pull request for now please. |
jillemash
dismissed
sebastien-hertz’s stale review
There's more check to be made before merging this. I already see some issues with this approach.
|
This does not work as it requires a repo to have been previously cloned, which is not yet the case before any task has been executed.
|
This way, checks on task form entries concerning branch or commit id can work.
Also make clone() return output of command, so error can be checked by caller.
| appError(req, res, '/bot/' + serviceName + ', repository of ' + repoName + ' could not be cloned'); | ||
| return; | ||
| } | ||
| var fetchOut = gitForBot.fetch(); |
There was a problem hiding this comment.
IMO the fetch part should be in the else clause of the if. I mean you only need to 'git fetch' if the repo already exists (and you want to update remote info), otherwise you'd do the 'git clone' which would give an up-to-date repo.
There was a problem hiding this comment.
I agree, although the fetch part should take no time if the repo just got cloned, it makes more sense that way. Done in upcoming commit.
sebastien-hertz
left a comment
sebastien-hertz
left a comment
There was a problem hiding this comment.
It seems to me that we should do either a 'git clone' or a 'git fetch' but not both. WDYT?
2 participants
Fixes #30