Skip to content

Conversation

@kriswest
Copy link
Contributor

@kriswest kriswest commented Oct 8, 2025

resolves #1237
resolves #1213

  • Removes the api.github config element as it is defunct
  • Adds config schema for commitConfig, attestationConfig and domains
  • Regenerates the config docs and types from the schema
  • Resolves a couple of type errors revealed by the updated types in scanDiff and checkCommitMessages
  • Removes the UserSettings type as this is no longer used (generated config type GitProxyConfig replaced it)

@netlify
Copy link

netlify bot commented Oct 8, 2025

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit cdcf900
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/68ee7c2945c71b0008878482

@kriswest kriswest changed the title Config schema for missing properties feat: config schema for missing properties Oct 9, 2025
@kriswest
Copy link
Contributor Author

kriswest commented Oct 9, 2025

At least two tests will not pass on this PR until its merged as they use config from main that contains invalid properties (mostly api.github, butattestationConfig with questions that have unsupported properties). We could relax the schema to allow these, but I don't think we should...

Edit: cofnirmed - although a gitrleaks test is also failing, I think thats unrelated to this PR.

@kriswest
Copy link
Contributor Author

kriswest commented Oct 9, 2025

I've manually run the Cypress tests, CLI tests and check-types, all of which pass (check-types still shows issues in the UI but they are unrelated to this PR).

@kriswest
Copy link
Contributor Author

kriswest commented Oct 9, 2025

@jescalada I'm not sure I see a way forward other than ignoring the check and merging. The reliance on the main branch for the configLoader tests mean they will always fail if we change the schema/default config such that the schema won't validate the old version...

@kriswest
Copy link
Contributor Author

kriswest commented Oct 9, 2025

@jescalada actually there is an answer. I've raised #1244 which corrects the config used in checks and should allow the two failing config tests on #1243 to pass if this is merged first. Also fixes a typo that was being checked in a test for gitleaks.

@jescalada
Copy link
Contributor

jescalada commented Oct 14, 2025

It seems some tests that load the proxy.config.json with the old values are failing. These will likely work after merging, as you mentioned 👍🏼

@codecov
Copy link

codecov bot commented Oct 14, 2025

Codecov Report

❌ Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.84%. Comparing base (847983d) to head (cdcf900).
⚠️ Report is 14 commits behind head on main.

Files with missing lines Patch % Lines
src/proxy/processors/push-action/scanDiff.ts 0.00% 0 Missing and 3 partials ⚠️
.../proxy/processors/push-action/checkAuthorEmails.ts 0.00% 0 Missing and 2 partials ⚠️
...roxy/processors/push-action/checkCommitMessages.ts 0.00% 0 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1243      +/-   ##
==========================================
- Coverage   84.07%   83.84%   -0.24%     
==========================================
  Files          68       68              
  Lines        2945     2946       +1     
  Branches      375      380       +5     
==========================================
- Hits         2476     2470       -6     
  Misses        409      409              
- Partials       60       67       +7     

☔ 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

It seems some tests that load the proxy.config.json with the old values are failing. These will likely work after merging, as you mentioned 👍🏼

Those tests are indeed now passing.

Re: coverage, we've got a handful of new uncovered partials (nullish coealscing to handle missing config entries). Thats never been a problem as the default config that we merge with has values set:

"api": {},
"commitConfig": {
"author": {
"email": {
"local": {
"block": ""
},
"domain": {
"allow": ".*"
}
}
},
"message": {
"block": {
"literals": [],
"patterns": []
}
},
"diff": {
"block": {
"literals": [],
"patterns": [],
"providers": {}
}
}
},
"attestationConfig": {
"questions": [
{
"label": "I am happy for this to be pushed to the upstream repository",
"tooltip": {
"text": "Are you happy for this contribution to be pushed upstream?",
"links": []
}
}
]
},

@kriswest kriswest enabled auto-merge October 14, 2025 16:36
@kriswest kriswest merged commit a386f0d into finos:main Oct 14, 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

Development

Successfully merging this pull request may close these issues.

Update config schema to govern and document config properties not currently captured Does anyone use GitHub api baseUrl

2 participants