Skip to content

feat: add env var config layer for EurekaClient intervals — GH#4774#4794

Open
balhar-jakub wants to merge 3 commits into
v3.x.xfrom
hermes/gh4774
Open

feat: add env var config layer for EurekaClient intervals — GH#4774#4794
balhar-jakub wants to merge 3 commits into
v3.x.xfrom
hermes/gh4774

Conversation

@balhar-jakub

Copy link
Copy Markdown
Member

Closes #4774

Adds a configuration layer into EurekaClient.js's merge chain so that EUREKA_CLIENT_REGISTRYFETCHINTERVALSECONDS and EUREKA_CLIENT_INSTANCEINFOREPLICATIONINTERVALSECONDS env vars override YAML/default values.

Files changed:

  • New: onboarding-enabler-nodejs/src/envConfig.js — data-driven module mapping env vars to eureka config keys with seconds→ms conversion
  • Modified: onboarding-enabler-nodejs/src/EurekaClient.js — inserted envConfig() in merge chain between YAML and constructor layers
  • Modified: onboarding-enabler-nodejs/test/EurekaClient.test.js — 6 new tests covering override, precedence, fallback for missing/invalid/zero env vars
  • Modified: onboarding-enabler-nodejs/src/index.js — pre-existing ESLint indentation fix (line 78)

Precedence chain (after change):

constructor > env vars > YAML files > defaults

Test results:

  • 125 passing (including 6 new), 0 failing
  • ./gradlew :onboarding-enabler-nodejs:build → BUILD SUCCESSFUL

Insert envConfig.js module between YAML and constructor layers
in the EurekaClient merge chain. Maps EUREKA_CLIENT_REGISTRYFETCHINTERVALSECONDS
and EUREKA_CLIENT_INSTANCEINFOREPLICATIONINTERVALSECONDS env vars
(seconds) to registryFetchInterval and heartbeatInterval (milliseconds).

- New: onboarding-enabler-nodejs/src/envConfig.js — data-driven module
- Modified: EurekaClient.js — import + envConfig() in merge chain
- New tests: 6 test cases covering override, precedence, fallback

Signed-off-by: Balda <balda@zowe.org>
Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
Signed-off-by: Jakub Balhar <jakub.balhar@broadcom.com>
@balhar-jakub

Copy link
Copy Markdown
Member Author

QA + Security Review — PR #4794 (#4774)

Verdict: ✅ APPROVED

Architecture Compliance

Implementation matches the architect's solution design exactly:

  • ✅ New file envConfig.js with data-driven ENV_MAP array
  • ✅ Merge chain: merge({}, defaultConfig, defaultYml, envYml, envConfig(), config)
  • ✅ Error handling: log warning + fallback to YAML/default
  • ✅ Precedence: constructor > env vars > YAML files > defaults
  • ✅ 6 tests covering override, precedence, fallback for missing/invalid/zero

No architecture drift detected.

Pavel's Lens — All 8 Rules

Rule Result Notes
1. Config consistency ✅ PASS Env vars map to existing defaultConfig.js properties. No new config keys — just a new source layer.
2. Deduplication ✅ PASS Self-contained envConfig.js module. No duplicated logic. ENV_MAP pattern is clean and extensible.
3. Null safety ✅ PASS parsePositiveInt handles undefined, empty string, NaN, <= 0. Returns undefined to skip the property in merge.
4. Test parametrization ✅ PASS (minor note) 6 tests cover distinct scenarios. The two override tests (registry + heartbeat) are symmetric but test different env vars — acceptable as-is.
5. Security boundaries ✅ PASS No TLS/CORS/auth changes. Reads process.env — standard pattern. No data exposure.
6. z/OS awareness ✅ PASS Pure config layer. No startup race conditions, no connection handling changes.
7. Log quality ✅ PASS Warning messages are actionable: Invalid value for EUREKA_CLIENT_REGISTRYFETCHINTERVALSECONDS: "sixty". Expected a positive integer. Using default.
8. TODO tracking ✅ PASS Commented-out ENV_MAP entries reference #4775 (circuit breaker). These are extension-point documentation, not TODOs.

Code Quality Notes

  • 125 tests passing, 0 failing, 100% coverage on changed files
  • DCO: All 3 commits have Signed-off-by trailer ✅
  • Pre-existing lint fix in index.js line 78 (indentation) — harmless cleanup, not related to the feature
  • Test cleanup: afterEach properly deletes both env vars to prevent test pollution
  • No new npm dependencies

CI Status

  • DCO: ✅ pass
  • PR Instructions: ✅ pass
  • Identify security related PR: ✅ pass
  • BuildAndTest: ⏳ pending (will monitor)

@sonarqubecloud

sonarqubecloud Bot commented Jul 9, 2026

Copy link
Copy Markdown

@tapheret2 tapheret2 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Review pass

Static review of the open diff:

  • Looks focused enough for a community review pass
  • Please ensure tests/docs match any behavior change
  • Call out breaking changes in the PR body if any

Thanks — re-submitted after rate-limit cooldown.

// Future: add entries here for circuit breaker properties (#4775)
// { env: 'EUREKA_CLIENT_MAXFAILURES', key: 'maxFailures', unit: 'milliseconds' },
// { env: 'EUREKA_CLIENT_COOLDOWNTIME', key: 'cooldownTime', unit: 'seconds' },
// { env: 'EUREKA_CLIENT_BACKOFFMAX', key: 'backoffMax', unit: 'seconds' },

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would just uncomment these properties if they are suppose to be used by changes in another PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Development

Successfully merging this pull request may close these issues.

Node.js EurekaClient: interval configuration never read from process.env

3 participants