feat: http filtering #415
feat: http filtering #415
Conversation
✅ Deploy Preview for endearing-brigadeiros-63f9d0 canceled.
|
coopernetes
force-pushed
the
gh410-http-filtering
branch
from
cefeee8 to
7b21074
Compare
|
@coopernetes - can we resolve merge conflicts? I merged #409 which also had changes to the router in |
coopernetes
force-pushed
the
gh410-http-filtering
branch
from
7b21074 to
909e5fd
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #415 +/- ##
==========================================
+ Coverage 56.85% 57.15% +0.30%
==========================================
Files 39 39
Lines 1036 1055 +19
==========================================
+ Hits 589 603 +14
- Misses 447 452 +5 ☔ View full report in Codecov by Sentry. |
|
Oh, Codecov how we love you so. @coopernetes - happy to bump up the coverage before we merge? We should probably make the project level Action informational and the PR level breaking whilst we slowly bring up the coverage for the rest of the repository. |
Most of the required refactoring for getting additional coverage is not something I want to do in this PR. I would essentially have to rewrite the Express middleware & routing layer to abstract it into a set of testable functions. That's probably effort better spent on changes to move us to 2.x.
Let me see what codecov makes available in their configuration. I would like to preserve the PR comment for the report even if the status check isn't blocking builds. This gives needed feedback in the PR while we improve overall coverage. |
|
Looking at the details, I think we can retest this after #429 is merged. The "baseline" report is misreported against a different branch. https://app.codecov.io/gh/finos/git-proxy/tree/bugfix%2Fci-permissions |
|
@coopernetes - agree with your comment before last. #429 is now merged by the way 👌 |
|
@coopernetes - as long as we can keep a view of the coverage of suggested changes in the PR that is good enough for now. |
|
Looking at Codecov, it looks like it is already achieving this. We just don't want to return a failing state to CI. |
|
Now that #442 is merged, let me rebase again and retest. @JamieSlome are you good with the latest changes? Hoping to get this merged soon. |
There was a problem hiding this comment.
LGTM @coopernetes ❤️
Would like to get an idea of the coverage percentage but happy to merge in any case. Will keep my eyes open for your re-base.
- Accept is only set for certain git actions so refactored into one validation function - pass routes correctly to express (fix TypeError on startup)
- separate out the HTTP git filtering logic from the URL parsing, which includes stripping the GitHub repo owner/name.
coopernetes
force-pushed
the
gh410-http-filtering
branch
from
909e5fd to
3092274
Compare
|
* feat: filter non-git HTTP requests being proxied Fixes finos#410 * feat: refactor to one validation function - Accept is only set for certain git actions so refactored into one validation function - pass routes correctly to express (fix TypeError on startup) * feat: separate url stripping into its own function - separate out the HTTP git filtering logic from the URL parsing, which includes stripping the GitHub repo owner/name.
Fixes #410
(Hello from Git Proxy 👋)