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

"installation_repositories" webhook #834

Merged
merged 1 commit into from
Jul 7, 2017

Conversation

begedin
Copy link
Contributor

@begedin begedin commented Jun 30, 2017

Closes #826

This is a spike to get the most basic path for handling of the "installation_repositories" webhook working. The 3 basic cases work

  • added -> repo does not exist -> create repo and project repo
  • added -> repo exists -> create project repo
  • removed -> repo exists, project repo exists -> remove project repo, leave repo

These 3 paths are done and tests for them pass. However, there are details we need to iron out as well as edge cases we need to handle. They are all marked by TODO comments in the event module.

@begedin begedin changed the title Spike to get the event handling working "installation_repositories" webhook Jun 30, 2017
@begedin
Copy link
Contributor Author

begedin commented Jun 30, 2017

NOTE: One of the tests in the handler_test.exs is failing because the module is trying to delete a project github repo, but there is nothing there to delete.

@begedin begedin force-pushed the 826-installation-repositories-webhook branch 3 times, most recently from 51852b9 to 2609560 Compare July 3, 2017 13:32
@begedin
Copy link
Contributor Author

begedin commented Jul 3, 2017

I pushed this as far as I could. There are edge cases to be handled, but I believe they are out of scope for this PR. Here's a list. I'm currently creating issues for these:

  • We get an InstallationRepositories::added event, but there is no GithubAppInstallation on record. Should not be happening, unsure how to handle. Only case I can think of is something went wrong in handling the Installation::created event, or if we somehow deleted the installation when we should not have.
  • We get an InstallationRepositories::added event. There is a GithubAppInstallation, but there is no GithubRepo to create a ProjectGithubRepo for. This happens in one of the following cases
    • something went wrong when handling Installation::created and we did not create a GithubRepo during that process.
    • repository was created after the installation of the app. The owner is now adding the repo to the app. That means we should be handling the Repository::created and Repository::removed events. - Handle Repository::created event #835, Handle Repository::deleted event  #836
  • We get an InstallationRepositories::removed event, there is no GithubAppInstallation, or there is a GithubAppInstallation but no GithubRepo, or there are both, but no ProjectGithubRepo. None of these should be happening. If they do happen, it means we aren't handling some other important event. Repository::created::deleted from Handle Repository::created event #835, Handle Repository::deleted event  #836 are likely culprits, but we should also handle Installation::deleted as a possible source of "desync" - 837
  • We assume a single repo payload in the "repositories_added/removed" hash, reason being, I assumed that's the case we handle in our UI as well. However, we should support adding/removing multiples since

@begedin
Copy link
Contributor Author

begedin commented Jul 3, 2017

@joshsmith

Based on your feedback from standup, we create/delete ProjectGithubRepo records on user UI click from CodeCorps only.

From that, it's not completely clear what we do on InstallationRepositories::added::removed.

Is the end goal creation/removal of GithubRepo records?

If so, how do the Repository::created::deleted events (#835, #836) go into it? (need feedback there).

Is it not possible to "follow", "unfollow" repositories for an app from Github's own UI?

Also, if it's just a single click, without a select/deselect all, then #839 is way smaller in priority, possibly even not necessary.

This will certainly make things simpler in the module being added here, though.

@joshsmith
Copy link
Contributor

it's not completely clear what we do on InstallationRepositories::added::removed.

Is the end goal creation/removal of GithubRepo records?

Yes. However, in the removed case, if there are any ProjectGitHubRepo records related to the GithubRepo records being deleted, then we'll need to delete those ProjectGitHubRepo records, as well.

If so, how do the Repository::created::deleted events (#835, #836) go into it? (need feedback there).

I don't think we need to handle these. We're only concerned with the repositories available to a given installation.

Is it not possible to "follow", "unfollow" repositories for an app from Github's own UI?

It is possible. This is the only way repositories get added/removed to/from an installation, other than those repositories added in the initial install step (which may have been all).

Also, if it's just a single click, without a select/deselect all, then #839 is way smaller in priority, possibly even not necessary.

I think we'd prefer a single click for now. Maybe at some point we can do a select all, but the interactions are just too complex right now for us to concern ourselves with bundling all these up into a single action just yet.

@joshsmith
Copy link
Contributor

We get an InstallationRepositories::added event, but there is no GithubAppInstallation on record. Should not be happening, unsure how to handle. Only case I can think of is something went wrong in handling the Installation::created event, or if we somehow deleted the installation when we should not have.

It's entirely possible that an event is not yet processable because we don't have sufficient information to process it. We need to be able to mark the state of these events and loop back to them to reprocess once we have the data. There are numerous reasons that could lead to receiving and processing webhooks out of order, so we need to be resilient and idempotent.

@joshsmith
Copy link
Contributor

We get an InstallationRepositories::added event. There is a GithubAppInstallation, but there is no GithubRepo to create a ProjectGithubRepo for. This happens in one of the following cases

I think these questions are maybe resolved due to our conversation earlier this morning. You can correct me if I'm wrong.

@joshsmith
Copy link
Contributor

We get an InstallationRepositories::removed event, there is no GithubAppInstallation, or there is a GithubAppInstallation but no GithubRepo, or there are both, but no ProjectGithubRepo.

See my comment above about idempotency.

@begedin
Copy link
Contributor Author

begedin commented Jul 4, 2017

@joshsmith There's a small part that irks e a bit.

The "repositories_added"/"removed_lists" in the event payload only have ["id", "name", "full_name"] as keys, meaning we do not have a full set of data to create our GithubRepo record. One option would be to query the API for the full set, but I'm not sure which endpoint to query.

I guess https://developer.github.com/v3/repos/#get points us in the direction of GET /repos/:owner/:repo

However, in the payload, we do have the "sender" object, which is the repo owner in all the examples I'm finding. My question is, will the sender always be the owner, or can they ever be someone else? If it's always the owner, it greatly simplifies our process, since we have all the data we need.

From https://developer.github.com/v3/apps/installations/#add-repository-to-installation

The authenticated user must have admin access to the repository.

That would imply the sender could be an admin to, so no-go on that part.

However, the "installation" hash of the payload also has an "account" hash, which, again, contains information about the repo. Any chance that one is guaranteed to be the correct one?

If neither is right, we'd have to fetch from API, of course, but if it can be avoided, I would prefer to go one of these routes.

@begedin begedin force-pushed the 826-installation-repositories-webhook branch from 14f76b7 to 1c3d633 Compare July 4, 2017 10:41
def change do
create table(:project_github_repos) do
add :project_id, references(:projects, on_delete: :delete_all)
add :github_repo_id, references(:github_repos, on_delete: :delete_all)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

on_delete: specifies what happens when the parent record, in this case, GithubRepo or Project gets deleted, not what happens when we delete this record.

We want to delete a ProjectGithubRepo whenever a Project or a GithubRepo get deleted.

…" events

Make github repo adapter more strict. Add error response for invalid structure

Updated dialyxir

Add types to GithubRepo, GithubAppInstallation

Fixed some GitHub request issues and specs

Cleaned up Installation::created event handling
- restructured flow
- added transaction
- made sure all paths can execute without errors
@joshsmith joshsmith force-pushed the 826-installation-repositories-webhook branch from f8bd4c3 to fbe130b Compare July 7, 2017 19:28
@joshsmith joshsmith merged commit 602ef7f into develop Jul 7, 2017
@joshsmith joshsmith deleted the 826-installation-repositories-webhook branch July 7, 2017 19:32
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.

None yet

2 participants