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

Add GitHub pull request #1074

Merged
merged 2 commits into from
Oct 17, 2017
Merged

Add GitHub pull request #1074

merged 2 commits into from
Oct 17, 2017

Conversation

joshsmith
Copy link
Contributor

@joshsmith joshsmith commented Oct 17, 2017

What's in this PR?

LOC looks scary but 2,075 of those are just JSON fixtures.

Adds:

  • GithubPullRequest
  • Event handler, pull request linker and user linker, adapter, validator, changeset builder
  • Factory
  • Tests

This also adds a GithubPullRequest belongs_to to Task as our way of identifying whether a given Task is an issue or a pull request. There should never be a case where a Task has both.

References

Fixes #1073

@joshsmith
Copy link
Contributor Author

@begedin this was an awful lot of copypasta. I don't know how much of this deserves to be extracted out into similar places, but there does appear to be a fair amount of DRYing that could be done. Low priority, but perhaps we should loop back later to identify opportunities to clean this up?

@begedin
Copy link
Contributor

begedin commented Oct 17, 2017

I agree there should be plenty of things to dry up.

The various Linkers/Syncers, etc. act quite similarly across all events, so could maybe be moved into "Common"

begedin
begedin previously approved these changes Oct 17, 2017
Copy link
Contributor

@begedin begedin left a comment

Choose a reason for hiding this comment

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

Generally looks good, except we need a body parser. I'll start work on that.

defp find_or_init_task(
%ProjectGithubRepo{project_id: project_id, github_repo_id: github_repo_id},
%GithubPullRequest{id: github_pull_request_id}
) do
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't deal with the key part we need here - the parsing of the PR body to inferr which tasks are to be linked.

Really, it's the core logic of that that we need to work out, I think.

  • parse body for closing keywords, extract github ids
  • if they exist, find github issues with those ids, associate with tasks belonging to those issues, dissasociate form tasks belonging to other github issues
  • if none are found, disassociate from associated tasks which do belong to github issues

The special case is when we get a pr created which does not close any issues. In that case, my suggestion is we

  • create a codecorps task, which is linked to the pr, but not to a github iusse
  • we leave that task associated from now on, regardless of new associations being added/removed

My philosophy is, that one was created on CC and can potentially hold information that will be lost, if we disassociate. Maybe at some point in the future, we allow "converting the PR to a github issue", which would associate that task with a github issue as well. At that point, it can behave like a normal task, but until that point, we shouldn't assume and try to avoid any automatons with it. If the PR has been linked to an issue since, let the user close the original task we've created if they want to.

@begedin
Copy link
Contributor

begedin commented Oct 17, 2017

@joshsmith I added a body parser with the function we need. Howe we want to hook it in is not quite clear yet, so I didn't to approach that on my own. I did write up my suggestion above, though.

@joshsmith
Copy link
Contributor Author

Just a comment that it’s:

if they exist, find github issues and pull requests with those ids,

@joshsmith joshsmith merged commit 4ea97ae into develop Oct 17, 2017
@joshsmith joshsmith deleted the add-github-pull-request branch October 17, 2017 16:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a GithubPullRequest model and relationships
2 participants