Copilot commented on code in PR #764:
URL: https://github.com/apache/unomi/pull/764#discussion_r3304244916
##########
build.sh:
##########
@@ -797,13 +817,22 @@ if [ "$USE_MAVEN_CACHE" = false ]; then
MVN_OPTS="$MVN_OPTS -Dmaven.build.cache.enabled=false"
fi
+# Extra Maven options (e.g. CI matrix ports: -Delasticsearch.port=9400)
+if [ -n "${MAVEN_EXTRA_OPTS:-}" ]; then
+ MVN_OPTS="$MVN_OPTS $MAVEN_EXTRA_OPTS"
+fi
Review Comment:
`MAVEN_EXTRA_OPTS` is appended into a single string (`MVN_OPTS`) and later
expanded unquoted. This makes argument handling fragile
(word-splitting/globbing) and makes it hard to safely pass values containing
whitespace. Consider switching to a bash array for Maven arguments (e.g.,
`MVN_ARGS=(...)` and append extra opts as array entries) and invoking Maven
with array expansion.
##########
build.sh:
##########
@@ -38,12 +36,8 @@ handle_error() {
EOF
echo "Error occurred in:"
- echo " Command: $last_command"
echo " Line: $line_no"
echo " Exit code: $exit_code"
- if [ ! -z "$func_trace" ]; then
- echo " Function trace: $func_trace"
- fi
exit $exit_code
Review Comment:
`handle_error` no longer prints the failing command or any stack/context,
which makes CI failures harder to diagnose. To keep arguments small without
losing diagnostics, consider reading `BASH_COMMAND` inside `handle_error`
(instead of passing it through the trap) and printing a truncated value and/or
a short function trace.
##########
.github/workflows/unomi-ci-build-tests.yml:
##########
@@ -31,7 +31,7 @@ jobs:
sudo apt-get install -y graphviz
dot -V
- name: Build and Unit tests
- run: mvn -U -ntp -e clean install
+ run: ./build.sh --ci
Review Comment:
In CI (`--ci`) the workflow now runs `./build.sh`, which currently invokes
Maven twice (`mvn clean` then `mvn install`). This adds overhead compared to a
single `mvn clean install` and may noticeably increase CI duration. Consider
adding a CI fast-path in `build.sh` to run a single Maven invocation when
non-interactive/CI is enabled.
##########
itests/src/test/java/org/apache/unomi/itests/graphql/GraphQLListIT.java:
##########
@@ -77,15 +77,28 @@ public void testCRUD() throws Exception {
refreshPersistence(UserList.class);
- Thread.sleep(6000);
-
- try (CloseableHttpResponse response =
post("graphql/list/find-lists.json")) {
- final ResponseContext context =
ResponseContext.parse(response.getEntity());
-
- Assert.assertEquals(1, ((Integer)
context.getValue("data.cdp.findLists.totalCount")).intValue());
- Assert.assertEquals("testListId",
context.getValue("data.cdp.findLists.edges[0].node.id"));
- Assert.assertEquals(profile.getItemId(),
context.getValue("data.cdp.findLists.edges[0].node.active.edges[0].node.cdp_profileIDs[0].id"));
- }
+ final ResponseContext findListsContext = keepTrying("Failed
waiting for profile in list query",
+ () -> {
+ try (CloseableHttpResponse response =
post("graphql/list/find-lists.json")) {
+ return ResponseContext.parse(response.getEntity());
+ } catch (Exception e) {
+ return null;
+ }
+ },
+ context -> {
+ if (context == null) {
+ return false;
+ }
+ Integer totalCount = (Integer)
context.getValue("data.cdp.findLists.totalCount");
+ if (totalCount == null || totalCount != 1) {
+ return false;
+ }
+ Object profileId =
context.getValue("data.cdp.findLists.edges[0].node.active.edges[0].node.cdp_profileIDs[0].id");
+ return profile.getItemId().equals(profileId);
+ },
+ DEFAULT_TRYING_TIMEOUT, DEFAULT_TRYING_TRIES);
Review Comment:
The polling lambda swallows all exceptions and returns `null`, so real
failures (e.g., non-2xx responses, parse errors) get converted into a timeout
with no details. Consider at least logging the last exception (or storing it
and including it in the final assertion failure) to keep the test diagnosable
when the GraphQL call is genuinely broken.
##########
setenv.sh:
##########
@@ -17,8 +17,13 @@
# limitations under the License.
#
################################################################################
-export UNOMI_VERSION=`mvn
org.apache.maven.plugins:maven-help-plugin:2.1.1:evaluate
-Dexpression=project.version | grep -Ev '(^\[|Download\w+:)'`
-echo Detected project version=$UNOMI_VERSION
+# Quiet evaluate: avoid capturing Maven download lines into the environment
(breaks CI with ARG_MAX).
+export UNOMI_VERSION="$(mvn -B -q -DforceStdout help:evaluate
-Dexpression=project.version -DinteractiveMode=false 2>/dev/null)"
+if [ -z "$UNOMI_VERSION" ]; then
+ echo "Failed to detect project version from Maven" >&2
+ exit 1
+fi
Review Comment:
`mvn ... 2>/dev/null` suppresses all Maven error output. When version
detection fails, the script exits with a generic message but provides no root
cause, which makes CI/local debugging harder. Consider capturing stderr to a
temp file/variable and printing it on failure (or only filtering known noisy
download lines) so failures are actionable.
--
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]
