Skip to content

fix: rename schema field https_verify_cert to https_verify_certificate#438

Open
jarvis9443 wants to merge 1 commit intomainfrom
fix/healthcheck-https-verify-cert-name
Open

fix: rename schema field https_verify_cert to https_verify_certificate#438
jarvis9443 wants to merge 1 commit intomainfrom
fix/healthcheck-https-verify-cert-name

Conversation

@jarvis9443
Copy link
Copy Markdown
Contributor

@jarvis9443 jarvis9443 commented Apr 28, 2026

The active health check schema in libs/sdk/src/core/schema.ts and libs/backend-apisix-standalone/src/typing.ts defines the field as https_verify_cert, but the canonical name everywhere else is https_verify_certificate:

  • APISIX apisix/schema_def.lua and lua-resty-healthcheck-api7
  • Dashboard API and APISIX Admin API responses

Because the parent schema uses z.strictObject, adc dump of a service with an HTTPS active health check produces YAML that contains https_verify_certificate: true, and the very next adc sync / adc lint rejects it:

Unrecognized key: "https_verify_certificate"
  → at services[0].upstream.checks.active

Round-trip dump→sync is broken for any service using HTTPS active health checks. Reported in https://github.com/api7/api7ee-3-control-plane/issues/2629.

The existing e2e fixtures (libs/backend-api7/e2e/default-value.e2e-spec.ts, libs/differ/src/test/usecase.spec.ts) already use the correct https_verify_certificate, but they don't go through the lint stage so the typo wasn't caught.

This PR:

  • Renames the field in both schemas to https_verify_certificate
  • Regenerates schema.json
  • Adds a regression test in apps/cli/src/linter/specs/upstream.spec.ts

Verified locally with nx test cli (26 passed).

Summary by CodeRabbit

  • Bug Fixes

    • Updated the HTTPS certificate verification configuration field name in upstream health checks across validation schemas. Users with existing configurations should update their field references accordingly.
  • Tests

    • Added test coverage for HTTPS active health checks with certificate verification enabled.

The active health check schema in libs/sdk/src/core/schema.ts and
libs/backend-apisix-standalone/src/typing.ts used `https_verify_cert`,
but the canonical name in APISIX (apisix/schema_def.lua) and the
Dashboard API is `https_verify_certificate`. Because the parent
schema uses z.strictObject, `adc dump` of a service with an HTTPS
active health check produces YAML that `adc sync` (and `adc lint`)
then rejects with: Unrecognized key: "https_verify_certificate".

Rename the schema field, regenerate schema.json, and add a regression
test to upstream linter spec.
@jarvis9443 jarvis9443 requested a review from bzp2010 as a code owner April 28, 2026 03:50
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 28, 2026

📝 Walkthrough

Walkthrough

This PR renames the HTTPS health check certificate verification configuration field from https_verify_cert to https_verify_certificate across test files, TypeScript Zod schemas (backend and SDK), and the JSON schema. Field behavior, constraints, and defaults remain unchanged. A new test case validates the updated field name.

Changes

Cohort / File(s) Summary
Health Check Configuration Test
apps/cli/src/linter/specs/upstream.spec.ts
Adds new test case verifying HTTPS active health check with https_verify_certificate: true field validation.
Health Check Schema Definitions
libs/backend-apisix-standalone/src/typing.ts, libs/sdk/src/core/schema.ts, schema.json
Renames HTTPS certificate verification field from https_verify_cert to https_verify_certificate across all Zod and JSON schema definitions while maintaining boolean type, optional status, and true default value.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~15 minutes

Suggested labels

test/api7

Suggested reviewers

  • LiteSun
  • guoqqqi
🚥 Pre-merge checks | ✅ 5 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
E2e Test Quality Review ⚠️ Warning Test is a unit/schema validation test that only verifies the linter accepts the field in isolation, not the full dump→lint→sync round-trip described in PR objectives. Add an E2E test exercising the complete dump→lint→sync round-trip with HTTPS active health checks and https_verify_certificate to verify the fix solves the reported issue.
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and accurately summarizes the main change: renaming a schema field from https_verify_cert to https_verify_certificate across multiple configuration files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Security Check ✅ Passed Safe schema field rename from https_verify_cert to https_verify_certificate with secure default maintained and no vulnerabilities introduced.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/healthcheck-https-verify-cert-name

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
apps/cli/src/linter/specs/upstream.spec.ts (1)

96-130: Add a complementary negative case for the deprecated key.

Nice positive regression at Line 97. Consider adding one case with https_verify_cert expecting failure so the strict-key rejection is explicitly locked in.

Proposed test addition
+    {
+      name: 'should reject deprecated https_verify_cert on active health check',
+      input: {
+        services: [
+          {
+            name: 'HealthCheck_HTTPS_Deprecated_VerifyCert',
+            upstream: {
+              nodes: [
+                {
+                  host: '1.1.1.1',
+                  port: 443,
+                  weight: 100,
+                },
+              ],
+              checks: {
+                active: {
+                  type: 'https',
+                  http_path: '/',
+                  https_verify_cert: true,
+                },
+              },
+            },
+          },
+        ],
+      } as unknown as ADCSDK.Configuration,
+      expect: false,
+    },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@apps/cli/src/linter/specs/upstream.spec.ts` around lines 96 - 130, Add a
negative test case mirroring the positive case "should accept
https_verify_certificate on active health check" that uses the deprecated key
name https_verify_cert (e.g., name it "should reject https_verify_cert on active
health check") so the linter enforces strict keys; create a new test input that
is identical to the existing service object (service name like
"HealthCheck_HTTPS_VerifyCert") but in the upstream.checks.active object replace
https_verify_certificate with https_verify_cert and set expect: false, and
assert the validation failure via the same test harness used by the surrounding
spec so the suite explicitly fails on the deprecated key.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@apps/cli/src/linter/specs/upstream.spec.ts`:
- Around line 96-130: Add a negative test case mirroring the positive case
"should accept https_verify_certificate on active health check" that uses the
deprecated key name https_verify_cert (e.g., name it "should reject
https_verify_cert on active health check") so the linter enforces strict keys;
create a new test input that is identical to the existing service object
(service name like "HealthCheck_HTTPS_VerifyCert") but in the
upstream.checks.active object replace https_verify_certificate with
https_verify_cert and set expect: false, and assert the validation failure via
the same test harness used by the surrounding spec so the suite explicitly fails
on the deprecated key.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 45995db7-d427-48e2-a987-9d9e4a5f2d00

📥 Commits

Reviewing files that changed from the base of the PR and between a3b2610 and a79b54f.

📒 Files selected for processing (4)
  • apps/cli/src/linter/specs/upstream.spec.ts
  • libs/backend-apisix-standalone/src/typing.ts
  • libs/sdk/src/core/schema.ts
  • schema.json

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant