Skip to content

refactor(config): drive deprecation warnings from config.schema.json - #1708

Merged
kriswest merged 4 commits into
finos:mainfrom
amitvijapur:fix/schema-driven-deprecation-warnings
Sep 14, 2026
Merged

kriswest merged 4 commits into
finos:mainfrom
amitvijapur:fix/schema-driven-deprecation-warnings

Conversation

@amitvijapur

Copy link
Copy Markdown
Contributor

Description

getDeprecatedConfigWarnings hardcoded one check per deprecated field, while config.schema.json already marked the same three fields "deprecated": true. The two had to be kept in step by hand, so adding a deprecation to the schema produced no warning until someone remembered to edit deprecatedFields.ts as well.

Warnings are now generated from the schema. Marking a property "deprecated": true is sufficient; x-deprecated-replacement names the field that supersedes it and selects the "use X instead" wording, and its absence gives the "deprecated and ignored" wording used by proxyUrl.

Warning text is unchanged for all three existing fields.

To be clear about what this is: it is a refactor, not a bug fix. Behaviour for the current three fields is identical, which is why the pre-existing tests pass untouched. The change is that a fourth deprecation now needs no code. I checked that concretely by temporarily marking a fourth property deprecated in the schema: the new code emits a warning, the old code returns [].

The loop reads top-level properties only. There are no nested deprecations today, and a deprecation nested under tls or configurationSources would not be picked up.

Related Issue

Resolves #1680

Checklist

General

Documentation

  • Documentation has been added/updated for any new features — N/A, no user-facing behaviour change

Configuration

  • If configuration schema (config.schema.json) was modified:
    • TypeScript types regenerated (npm run generate-config-types) — output is byte-identical, quicktype ignores the x- keyword
    • Schema reference docs regenerated (npm run gen-schema-doc) — see note below

Tests

  • Tests have been added/updated for new functionality

Notes for the reviewer

On gen-schema-doc. I ran it and confirmed my schema change produces a byte-identical reference.mdx, since json-schema-for-humans ignores the custom keyword. I have deliberately not committed the regenerated file, because running it also pulls in an unrelated sidebandProgress section: that property is present in config.schema.json on main but missing from the committed website/docs/configuration/reference.mdx, so the doc is already stale independently of this PR. Happy to include that regeneration here if you would rather, but it looked like it belongs in its own commit.

On the two new tests. Neither the no-replacement branch (proxyUrl) nor the suppression branch (deprecated field set and its replacement set) had coverage before this PR. Both are covered now.

On _.get. src/config/validators.ts has an equivalent dotted-path lookup, but it is a local closure inside validateConfigRegex rather than an exported helper, so reusing it would have meant refactoring that function too. I used lodash.get instead, since lodash is already a direct dependency used elsewhere in src/. Happy to extract and share the existing helper instead if you prefer one implementation.

Disclosure. This PR was written with AI assistance. I understand and can justify the change end to end; the points I am least certain about are flagged above (the top-level-only scope, and the _.get choice).

`getDeprecatedConfigWarnings` hardcoded a check per deprecated field while
`config.schema.json` already marked the same three fields `"deprecated": true`,
so the two had to be kept in step by hand.

The warnings are now generated from the schema. Adding `"deprecated": true` to
a property is enough to produce a warning; `x-deprecated-replacement` names the
field that supersedes it, and its absence produces the "deprecated and ignored"
wording instead.

Warning text is unchanged, and quicktype ignores the custom keyword, so
`src/config/generated/config.ts` regenerates byte for byte.

Closes finos#1680
@amitvijapur
amitvijapur requested a review from a team as a code owner August 31, 2026 19:30
@linux-foundation-easycla

linux-foundation-easycla Bot commented Aug 31, 2026

Copy link
Copy Markdown

CLA Signed
The committers listed above are authorized under a signed CLA.

  • ✅ login: amitvijapur / name: Amit Vijapur (f0038db)

@netlify

netlify Bot commented Aug 31, 2026

Copy link
Copy Markdown

Deploy Preview for endearing-brigadeiros-63f9d0 canceled.

Name Link
🔨 Latest commit fd2d7f7
🔍 Latest deploy log https://app.netlify.com/projects/endearing-brigadeiros-63f9d0/deploys/6aa531986320810008105ef7

@codecov

codecov Bot commented Sep 1, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 86.10%. Comparing base (c4e2107) to head (fd2d7f7).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1708      +/-   ##
==========================================
+ Coverage   86.05%   86.10%   +0.05%     
==========================================
  Files         101      101              
  Lines        5571     5577       +6     
  Branches      995      996       +1     
==========================================
+ Hits         4794     4802       +8     
+ Misses        526      525       -1     
+ Partials      251      250       -1     

☔ View full report in Codecov by Harness.
📢 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.

@jescalada jescalada left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution @amitvijapur! Looks good after applying my suggestion.

Do you mind filling in EasyCLA in order to get this merged?

Comment thread src/config/deprecatedFields.ts Outdated
Applies review feedback from @jescalada. The previous guard only counted
strings as set, so a non-string legacy value (or a non-string replacement)
was silently ignored instead of warned about.

Signed-off-by: Amit Vijapur <vijapuramit@gmail.com>
@amitvijapur

Copy link
Copy Markdown
Contributor Author

Applied the suggestion in 14c7270, thanks. Added a test covering the non-string case so the widened behaviour is pinned. EasyCLA is signed and now passing.

@amitvijapur

Copy link
Copy Markdown
Contributor Author

@jescalada when you have a moment — your suggestion is applied in 14c72702, EasyCLA is signed and passing, and all checks are green. Only the review state from 1 September is still outstanding.

(GitHub does not let me send a re-review request from a fork, hence the comment.)

@amitvijapur

Copy link
Copy Markdown
Contributor Author

Gentle follow-up on this one. Everything on my side is done:

  • @jescalada's suggestion applied in 14c72702
  • EasyCLA passing ("You are authorized to contribute")
  • All checks green, branch mergeable, no conflicts

The only thing outstanding is the review state from 1 September, which predates the change that addressed it. I can't send a re-review request from a fork, so I'm flagging it here instead.

@jescalada if you're busy, would any other maintainer be able to take a look? Happy to rebase or adjust anything if it's gone stale.

@kriswest kriswest left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@kriswest
kriswest dismissed jescalada’s stale review September 11, 2026 13:07

Suggestion applied

@kriswest
kriswest enabled auto-merge (squash) September 11, 2026 13:08
@amitvijapur

Copy link
Copy Markdown
Contributor Author

Thanks for the review @kriswest.

Merged current main in fd2d7f75 to clear the BEHIND state, which also re-triggers CI. The branch was 24 commits behind after the root-level cleanup in #1712, which moved the vitest configs; our tests still pass under the new test/vitest.config.ts (5 passed).

On the red checks from the previous run: the build-windows failure was test/proxy/performance.test.ts > Error Handling Performance > should handle errors quickly without memory leaks. This PR only touches config.schema.json, src/config/deprecatedFields.ts and its test, so it does not reach proxy performance or memory. #1726 also fails build-windows but on a different test (test/ssh/security.test.ts), which suggests the Windows runner rather than either change. Flagging in case it is worth a look separately.

Disclosure: written with AI assistance, reviewed by me before posting.

@amitvijapur

Copy link
Copy Markdown
Contributor Author

One thing needs a maintainer: the workflow runs on fd2d7f75 are all sitting at action_required, so CI has not actually executed on the merge commit.

CI                    action_required
E2E Tests             action_required
CodeQL                action_required
CVE Scanning          action_required
Code Cleanliness      action_required
Dependency Review     action_required
Package Smoke Test    action_required
Unused Dependencies   action_required

Only the seven checks that do not need approval have reported (EasyCLA, labelling, Netlify preview), which is why the PR shows BLOCKED despite the approval. Could someone approve the workflow run? @kriswest @jescalada

Nothing else outstanding as far as I can tell: approved, no conflicts, no longer behind, and our tests pass locally under the relocated test/vitest.config.ts (5 passed).

@kriswest
kriswest merged commit bdcccc6 into finos:main Sep 14, 2026
34 of 36 checks passed
@amitvijapur
amitvijapur deleted the fix/schema-driven-deprecation-warnings branch September 15, 2026 15:47
dcoric added a commit that referenced this pull request Sep 16, 2026
Brings in the config deprecation refactor (#1708) and the agent-base
and c8 dependency bumps. Clean merge, no conflicts.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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.

Pull deprecated status from config.schema.json, make warnings generic

3 participants