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

Ignore repo syncs for projects without project id. #1254

Merged
merged 1 commit into from
Nov 27, 2017
Merged

Conversation

asummers
Copy link
Contributor

@asummers asummers commented Nov 26, 2017

Fixes #1221

@joshsmith
Copy link
Contributor

Just note to self that you're working this to:

  • Add a regression test
  • Fix the existing test failures

@asummers
Copy link
Contributor Author

Things to consider moving forward:

There are "classes" of webhooks, some of which pertain to the repository and project and other that don't (installation, e.g.). As CC adds more webhook handlers for various reasons, the should_process? function will become increasingly further away from the handler logic, so this should eventually move down into the event handler logic, with the controller merely being a dispatcher.

|> process_event(type, delivery_id, payload)
process_status = type |> EventSupport.status(action)
process_status |> process_event(type, delivery_id, payload)
process_status
else
:ignored
end
Copy link
Contributor

Choose a reason for hiding this comment

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

So will this line right now never run, the :ignored line?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I get it with the above comment. Should've waited.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The :ignored line will be executed in the case where the webhook event includes %{"repository" => %{"id" => _}} but the associated GithubRepo does not contain an associated project_id in the DB.

Copy link
Contributor

@joshsmith joshsmith left a comment

Choose a reason for hiding this comment

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

One minor change but otherwise LGTM.

where: not(is_nil(repo.project_id))
Repo.one(query) != nil
end
def should_process?(_), do: true
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to make this fn private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We absolutely do, good catch.

@joshsmith
Copy link
Contributor

With that change and a squash into a single commit we're good to merge this.

@joshsmith joshsmith merged commit 190f295 into develop Nov 27, 2017
@joshsmith joshsmith deleted the issue-1221 branch November 27, 2017 00:38
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