fix(webhook): move skip check to after hook creation

[ecrupper]

Moving this skip check down will relay skipped message to the users in the audit tab rather than burying it in the webhook API responses.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 58.15%. Comparing base (3572fd8) to head (18290a0).

Files with missing lines	Patch %	Lines
api/webhook/post.go	0.00%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1366      +/-   ##
==========================================
- Coverage   58.15%   58.15%   -0.01%     
==========================================
  Files         634      634              
  Lines       24589    24591       +2     
==========================================
  Hits        14300    14300              
- Misses       9639     9641       +2     
  Partials      650      650              

Files with missing lines	Coverage Δ
api/webhook/post.go	0.00% <0.00%> (ø)

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wass3rw3rk]

not a fan of this solution, we're now potentially doing a whole lot of work for a hook that won't produce a build, aren't we?

[ecrupper]

not a fan of this solution, we're now potentially doing a whole lot of work for a hook that won't produce a build, aren't we?

we do the same for events not allowed, builds that get skipped due to no matching rulesets, and repos that are no longer active — all of which happen far more frequently

[wass3rw3rk]

we do the same for events not allowed, builds that get skipped due to no matching rulesets, and repos that are no longer active — all of which happen far more frequently

sure - we should add a story to deal with those (and any other) scenarios where we do unnecessary work 🙈

[ecrupper]

we do the same for events not allowed, builds that get skipped due to no matching rulesets, and repos that are no longer active — all of which happen far more frequently

sure - we should add a story to deal with those (and any other) scenarios where we do unnecessary work 🙈

Yeah but ultimately we are relaying information to the user with some sort of stateful object (Hook) to show in the audit tab. All that takes place beforehand is the following:

1. Parse webhook payload and hydrate data for repo, hook, build
2. Fetch repo to verify hook with hash
3. Update repo with topics, custom props, default branch
4. Create hook

I'd argue all of those are necessary except maybe 3. I am of the opinion that whenever our application is rejecting / skipping a build, it should show up in the audit tab.

[wass3r]

I am of the opinion that whenever our application is rejecting / skipping a build, it should show up in the audit tab.

Totally, the goal of the PR is great and needed!

[wass3r]

In the spirit of moving forward, I agree that the only change that probably makes sense is to skip UpdateRepo when the build is skipped. Even better probably, is to only call UpdateRepo if there are actually changes.