Skip to content

Conversation

kriswest
Copy link
Contributor

@kriswest kriswest commented Apr 15, 2025

resolves #948
resolves #947

Fixes a number of shortcomings in the file DB implementation, including:

  • Merge documents when updating user records instead of replacing (to match behaviour of mongo - was dropping gitAccount when record was updated)
  • Apply basic validation to repository records (to match behaviour of mongo)
  • Reduce username and email to lowercase consistently in DB clients
  • Add indexes and compaction settings (db file format is append only and needs periodic compaction)

Changes were also applied to the mongo DB classes:

  • Be more consistent about lowercasing user details (username and email)
  • Lowercase repo name consistently
    • this change may be controversial (may need migration)
    • however a number of functions already lowercase the repo name... e.g. addUserCanPush and addUserCanAuthorise, its just not being applied consistently

Finally, tests were updates:

  • existing was updated to ensure it uses unique usernames and emails as this is necessary after the indexes are added.
  • Many more DB tests were introduced. These only cover the fileDB currently, but could be run over mongo implementation if an instance is setup for testing and config applied, perhaps in a GH action

kriswest added 21 commits April 10, 2025 13:16
@kriswest kriswest changed the title 948 neDB implementation issues fix: neDB implementation issues Apr 15, 2025
Copy link

netlify bot commented Apr 15, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit b10763f
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/684055e3d56ccc0008c39dc7

@github-actions github-actions bot added the fix label Apr 15, 2025
Copy link

codecov bot commented Apr 15, 2025

Codecov Report

Attention: Patch coverage is 77.11864% with 27 lines in your changes missing coverage. Please review.

Project coverage is 60.15%. Comparing base (7c3bd62) to head (b10763f).
Report is 40 commits behind head on main.

Files with missing lines Patch % Lines
src/plugin.ts 6.25% 15 Missing ⚠️
src/service/passport/oidc.js 0.00% 4 Missing ⚠️
.../proxy/processors/push-action/checkAuthorEmails.ts 0.00% 2 Missing ⚠️
src/db/file/repo.ts 96.42% 0 Missing and 1 partial ⚠️
src/proxy/actions/Step.ts 75.00% 1 Missing ⚠️
src/proxy/processors/pre-processor/parseAction.ts 50.00% 1 Missing ⚠️
src/proxy/processors/push-action/preReceive.ts 0.00% 1 Missing ⚠️
src/proxy/processors/push-action/pullRemote.ts 66.66% 1 Missing ⚠️
src/proxy/processors/push-action/scanDiff.ts 94.73% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #979      +/-   ##
==========================================
+ Coverage   53.80%   60.15%   +6.35%     
==========================================
  Files          53       53              
  Lines        2171     2196      +25     
  Branches      244      250       +6     
==========================================
+ Hits         1168     1321     +153     
+ Misses        952      840     -112     
+ Partials       51       35      -16     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@kriswest
Copy link
Contributor Author

This PR has one more push to come that will hopefully complete the code coverage. Should be reviewed and pushed tomorrow

@kriswest
Copy link
Contributor Author

One last commit to push on this PR that should get coverage on /src/db/file up to near 100% - might get pushed after easter now.

@coopernetes coopernetes self-assigned this Apr 21, 2025
@kriswest
Copy link
Contributor Author

@JamieSlome @coopernetes @grovesy this PR should be ready to go now

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

@kriswest - before I review pre-merge, can we confirm that the linting changes are aligned with the current config and expectation asserted by the project?

@kriswest
Copy link
Contributor Author

kriswest commented May 7, 2025

@JamieSlome I was set-up as per the project config, however the project config doesn't cover packages/git-proxy-cli! I've tweaked the format command so that it does apply and run it. I tried to add prettier to the child project directly but it's not working there, so I extended the parent project's format command to cover it instead.

This adds a bunch of files to the PR with minor changes - waiting on a review then will push

@kriswest
Copy link
Contributor Author

kriswest commented May 7, 2025

@JamieSlome prettier was run on everything and should be applying to packages/git-proxy-cli. It looks like that was previously formatted with different settings / narrower width. Does it look right to you?

Copy link
Contributor

@coopernetes coopernetes left a comment

Choose a reason for hiding this comment

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

These are all welcome changes and LGTM on the database side. I'm leaving the UI review to @JamieSlome

@kriswest
Copy link
Contributor Author

kriswest commented Jun 3, 2025

Conflict resolutions on their way...

@kriswest
Copy link
Contributor Author

kriswest commented Jun 4, 2025

Conflicts resolved

Copy link
Contributor

@sam-holmes2 sam-holmes2 left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution :)

Copy link
Member

@JamieSlome JamieSlome left a comment

Choose a reason for hiding this comment

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

LGTM! 🍰

@JamieSlome JamieSlome enabled auto-merge June 4, 2025 14:21
@JamieSlome JamieSlome merged commit a3834ab into finos:main Jun 4, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants