fix(upstreams): unable remove some fields' value [KM-970] #1978

raoxiaoyan · 2025-02-24T07:18:56Z

.

Summary

KM-970
FTI-6493

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue

kongponents-bot · 2025-03-11T07:19:35Z

Preview components from this PR in consuming application

In consuming application project install preview versions of shared packages generated by this PR:

@kong-ui-public/entities-upstreams-targets@pr-1978

sumimakito

Thanks for the updates

sumimakito · 2025-03-11T08:57:36Z

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue

-  if (state.fields.healthchecksThreshold) {
-    result.healthchecks.threshold = Number(state.fields.healthchecksThreshold)
-  }
+const getBasePayload = computed((): UpstreamFormPayload => ( {


Suggested change

const getBasePayload = computed((): UpstreamFormPayload => ( {

const basePayload = computed((): UpstreamFormPayload => ({

sumimakito · 2025-03-11T08:57:59Z

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue

+})
+
+const getPayload = computed((): UpstreamFormPayload => {
+  const basePayload = getBasePayload.value


Suggested change

const basePayload = getBasePayload.value

sumimakito · 2025-03-11T08:58:43Z

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue

+  const payload: UpstreamFormPayload = {
+    ...basePayload,
+    healthchecks: {
+      ...basePayload.healthchecks,
+      active: active || undefined,
+      passive: passive || undefined,
+    },
+  }


Suggested change

const payload: UpstreamFormPayload = {

...basePayload,

healthchecks: {

...basePayload.healthchecks,

active: active || undefined,

passive: passive || undefined,

},

}

const payload: UpstreamFormPayload = {

...basePayload.value,

healthchecks: {

...basePayload.value.healthchecks,

active: active || undefined,

passive: passive || undefined,

},

}

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue

sumimakito · 2025-03-11T10:08:16Z

packages/entities/entities-upstreams-targets/src/composables/useHelpers.ts

+        healthy: {
+          interval: response?.healthchecks.active?.healthy?.interval?.toString() || '0',
+          successes: response?.healthchecks.active?.healthy?.successes?.toString() || '5',
+          httpStatuses: response?.healthchecks.active?.healthy?.http_statuses
+            ? numberToStringArray(response.healthchecks.active.healthy.http_statuses)
+            : ActiveHealthyHttpStatuses,
+        },
+        unhealthy: {
+          interval: response?.healthchecks.active?.unhealthy?.interval?.toString() || '0',
+          httpFailures: response?.healthchecks.active?.unhealthy?.http_failures?.toString() || '5',
+          tcpFailures: response?.healthchecks.active?.unhealthy?.tcp_failures?.toString() || '5',
+          httpStatuses: response?.healthchecks.active?.unhealthy?.http_statuses
+            ? numberToStringArray(response.healthchecks.active.unhealthy.http_statuses)
+            : ActiveUnhealthyHttpStatuses,
+          timeouts: response?.healthchecks.active?.unhealthy?.timeouts?.toString() || '0',
+        },


Shall we populate the empty fields with the defaults in the schema?

healthy = { interval = 0, -- 0 = probing disabled by default http_statuses = { 200, 302 }, successes = 0, -- 0 = disabled by default }, unhealthy = { interval = 0, -- 0 = probing disabled by default http_statuses = { 429, 404, 500, 501, 502, 503, 504, 505 }, tcp_failures = 0, -- 0 = disabled by default timeouts = 0, -- 0 = disabled by default http_failures = 0, -- 0 = disabled by default }

sumimakito · 2025-03-11T10:08:41Z

packages/entities/entities-upstreams-targets/src/composables/useHelpers.ts

+        healthy: {
+          successes: response?.healthchecks.passive?.healthy?.successes?.toString() || '5',
+          httpStatuses: response?.healthchecks.passive?.healthy?.http_statuses
+            ? numberToStringArray(response.healthchecks.passive.healthy.http_statuses)
+            : PassiveHealthyHttpStatuses,
+        },
+        unhealthy: {
+          timeouts: response?.healthchecks.passive?.unhealthy?.timeouts?.toString() || '0',
+          httpFailures: response?.healthchecks.passive?.unhealthy?.http_failures?.toString() || '5',
+          tcpFailures: response?.healthchecks.passive?.unhealthy?.tcp_failures?.toString() || '5',
+          httpStatuses: response?.healthchecks.passive?.unhealthy?.http_statuses
+            ? numberToStringArray(response.healthchecks.passive.unhealthy.http_statuses)
+            : PassiveUnhealthyHttpStatuses,
+        },


healthy = { http_statuses = { 200, 201, 202, 203, 204, 205, 206, 207, 208, 226, 300, 301, 302, 303, 304, 305, 306, 307, 308 }, successes = 0, }, unhealthy = { http_statuses = { 429, 500, 503 }, tcp_failures = 0, -- 0 = circuit-breaker disabled by default timeouts = 0, -- 0 = circuit-breaker disabled by default http_failures = 0, -- 0 = circuit-breaker disabled by default }

sumimakito · 2025-03-11T10:15:13Z

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue

+    type: state.fields.activeHealthCheck.type,
+    healthy: {
+      interval: Number(state.fields.activeHealthCheck.healthy.interval || '0'),
+      successes: Number(state.fields.activeHealthCheck.healthy.successes || '5'),


Should we use 0 here?

sumimakito · 2025-03-11T10:15:25Z

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue

+    unhealthy: {
+      interval: Number(state.fields.activeHealthCheck.unhealthy.interval || '0'),
+      timeouts: Number(state.fields.activeHealthCheck.unhealthy.timeouts || '0'),
+      tcp_failures: Number(state.fields.activeHealthCheck.unhealthy.tcpFailures || '5'),


Should we default to 0 here?

Nice catch.

raoxiaoyan · 2025-03-11T10:41:07Z

packages/entities/entities-upstreams-targets/src/composables/useHelpers.ts

-        interval: response?.healthchecks.active?.healthy?.interval?.toString() || '0',
-        successes: response?.healthchecks.active?.healthy?.successes?.toString() || '5',
-        httpStatuses: response?.healthchecks.active?.healthy?.http_statuses
-          ? numberToStringArray(response.healthchecks.active.healthy.http_statuses)
-          : ActiveHealthyHttpStatuses,
-        unhealthyInterval: response?.healthchecks.active?.unhealthy?.interval?.toString() || '0',
-        httpFailures: response?.healthchecks.active?.unhealthy?.http_failures?.toString() || '5',
-        tcpFailures: response?.healthchecks.active?.unhealthy?.tcp_failures?.toString() || '5',
-        unhealthyHttpStatuses: response?.healthchecks.active?.unhealthy?.http_statuses
-          ? numberToStringArray(response.healthchecks.active.unhealthy.http_statuses)
-          : ActiveUnhealthyHttpStatuses,
-        unhealthyTimeouts: response?.healthchecks.active?.unhealthy?.timeouts?.toString() || '0',


I don't know why here didn't populate the default value from the schema.
Maybe there is something wrong. Let me check it again.
@sumimakito

raoxiaoyan · 2025-03-11T10:49:21Z

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue

-  state.fields.activeHealthCheck.interval = val ? '5' : '0'
-  state.fields.activeHealthCheck.successes = val ? '5' : '0'
-  state.fields.activeHealthCheck.httpFailures = val ? '5' : '0'
-  state.fields.activeHealthCheck.unhealthyInterval = val ? '5' : '0'
-  state.fields.activeHealthCheck.tcpFailures = val ? '5' : '0'
+  state.fields.activeHealthCheck.healthy.interval = val ? '5' : '0'
+  state.fields.activeHealthCheck.healthy.successes = val ? '5' : '0'
+  state.fields.activeHealthCheck.healthy.httpStatuses = val ? ActiveHealthyHttpStatuses : []
+  state.fields.activeHealthCheck.unhealthy.httpStatuses = val ? ActiveUnhealthyHttpStatuses : []
+  state.fields.activeHealthCheck.unhealthy.httpFailures = val ? '5' : '0'
+  state.fields.activeHealthCheck.unhealthy.interval = val ? '5' : '0'
+  state.fields.activeHealthCheck.unhealthy.tcpFailures = val ? '5' : '0'
+  state.fields.activeHealthCheck.unhealthy.timeouts = val ? '5' : '0'
+  state.fields.activeHealthCheck.timeout = val ? '1' : '0'
+  state.fields.activeHealthCheck.concurrency = val ? '10' : '0'
+  state.fields.activeHealthCheck.httpPath = val ? '/' : ''
+  state.fields.activeHealthCheck.httpsSni = ''
+  state.fields.activeHealthCheck.type = 'http'
 }

 const resetOnPassiveSwitch = (val: boolean): void => {
-  state.fields.passiveHealthCheck.timeouts = val ? '5' : '0'
-  state.fields.passiveHealthCheck.successes = val ? '80' : '0'
-  state.fields.passiveHealthCheck.tcpFailures = val ? '5' : '0'
-  state.fields.passiveHealthCheck.httpFailures = val ? '5' : '0'


@2eha0 Why give these default values for these fields when enabling active or passive health?

Looking into the history of the file

I think it should be enabled the active health requires the non-zero number.
But Why give these numbers? Where can I find some docs to ref.

It seems the default values have been there since two years ago when it was first added. I also have no idea why we chose them but let's keep them there as the current values serve as a good simple example instead of the all-zero ones.

raoxiaoyan requested review from a team as code owners February 24, 2025 07:18

raoxiaoyan self-assigned this Feb 25, 2025

raoxiaoyan force-pushed the fix/KM-970 branch from f393a53 to 35e50a1 Compare February 25, 2025 07:43

raoxiaoyan changed the title ~~fix: fix(upstreams): unable remove some fields' value [KM-970]~~ fix(upstreams): unable remove some fields' value [KM-970] Feb 25, 2025

raoxiaoyan force-pushed the fix/KM-970 branch 4 times, most recently from b61d884 to 476efd5 Compare February 26, 2025 11:01

sumimakito reviewed Feb 27, 2025

View reviewed changes

packages/entities/entities-upstreams-targets/src/components/UpstreamsForm.vue Outdated Show resolved Hide resolved

raoxiaoyan force-pushed the fix/KM-970 branch 3 times, most recently from 2e1cadc to d27daac Compare March 3, 2025 02:51

raoxiaoyan requested a review from sumimakito March 3, 2025 02:54

raoxiaoyan force-pushed the fix/KM-970 branch from d27daac to 59dab33 Compare March 3, 2025 06:47

sumimakito reviewed Mar 3, 2025

View reviewed changes

raoxiaoyan force-pushed the fix/KM-970 branch from 59dab33 to 56c5db5 Compare March 4, 2025 07:02

raoxiaoyan force-pushed the fix/KM-970 branch from 56c5db5 to 94f96f1 Compare March 11, 2025 07:15

fix(upstreams-targets): unable remove some fields' value [KM-970]

bb5f876

raoxiaoyan force-pushed the fix/KM-970 branch from 94f96f1 to bb5f876 Compare March 11, 2025 07:17

raoxiaoyan requested a review from sumimakito March 11, 2025 08:09

sumimakito reviewed Mar 11, 2025

View reviewed changes

raoxiaoyan commented Mar 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(upstreams): unable remove some fields' value [KM-970] #1978

fix(upstreams): unable remove some fields' value [KM-970] #1978

raoxiaoyan commented Feb 24, 2025 •

edited

Loading

kongponents-bot commented Mar 11, 2025

sumimakito left a comment

sumimakito Mar 11, 2025

sumimakito Mar 11, 2025

sumimakito Mar 11, 2025

sumimakito Mar 11, 2025

sumimakito Mar 11, 2025

sumimakito Mar 11, 2025

sumimakito Mar 11, 2025

raoxiaoyan Mar 11, 2025

raoxiaoyan Mar 11, 2025

raoxiaoyan Mar 11, 2025 •

edited

Loading

sumimakito Mar 12, 2025

raoxiaoyan Mar 12, 2025

sumimakito Mar 12, 2025

	const getBasePayload = computed((): UpstreamFormPayload => ( {
	const basePayload = computed((): UpstreamFormPayload => ({

fix(upstreams): unable remove some fields' value [KM-970] #1978

Are you sure you want to change the base?

fix(upstreams): unable remove some fields' value [KM-970] #1978

Conversation

raoxiaoyan commented Feb 24, 2025 • edited Loading

Summary

kongponents-bot commented Mar 11, 2025

Preview components from this PR in consuming application

sumimakito left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raoxiaoyan Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raoxiaoyan commented Feb 24, 2025 •

edited

Loading

raoxiaoyan Mar 11, 2025 •

edited

Loading