Copilot commented on code in PR #13855:
URL: https://github.com/apache/apisix/pull/13855#discussion_r3861881664
##########
apisix/admin/standalone.lua:
##########
@@ -343,9 +346,12 @@ function _M.init_worker()
end
else
local last_modified = config[METADATA_LAST_MODIFIED]
- if last_modified_per_worker ~= last_modified then
+ local digest = config[METADATA_DIGEST]
+ if last_modified_per_worker ~= last_modified
+ or digest_per_worker ~= digest then
update_config(config)
Review Comment:
This change adds important behavior (digest-based change detection) but the
added test file only covers the “stream connection before first config” path
and doesn’t exercise the “same `X-Last-Modified`, different `X-Digest`”
scenario. Please add a focused test that simulates two updates with identical
`METADATA_LAST_MODIFIED` but different `METADATA_DIGEST`, and asserts
`update_config` is applied on the second update (e.g., by verifying
behavior/logs that depend on the updated config).
##########
t/admin/standalone-stream.t:
##########
@@ -0,0 +1,57 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements. See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+use t::APISIX 'no_plan';
+
+repeat_each(1);
+log_level('info');
+no_long_string();
+no_root_location();
+no_shuffle();
+
+add_block_preprocessor(sub {
+ my ($block) = @_;
+
+ if (!defined $block->yaml_config) {
+ $block->set_value("yaml_config", <<'_EOC_');
+deployment:
+ role: traditional
+ role_traditional:
+ config_provider: yaml
+ admin:
+ admin_key:
+ - name: admin
+ key: edd1c9f034335f136f87ad84b625c8f1
+ role: admin
+_EOC_
+ }
+
+ $block->set_value("stream_enable", 1);
+
+ if (!$block->stream_request) {
Review Comment:
`stream_request` is checked via truthiness (`!$block->stream_request`),
which will also treat valid-but-falsey values (e.g. `"0"` or `""`) as “not set”
and override them. Prefer an explicit definedness check (consistent with the
earlier `yaml_config` check) to avoid unintended overrides in future test
blocks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
