👻 Use exactly instead of atLeast in tests and fix tests wherever required#305
👻 Use exactly instead of atLeast in tests and fix tests wherever required#305pranavgaikwad merged 13 commits intokonveyor:mainfrom
Conversation
📝 WalkthroughWalkthroughLocal CI workflow updated: Java bumped to 21; large inline Block_Start/Block_End shell block added to start background cleanup, log-tail, and monitoring utilities; generated rules moved to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Runner as GitHub Actions runner
participant Shell as Workflow shell
participant Podman as Podman container
participant FS as Filesystem (~/.config/.kantra)
participant Cleaner as cleanup_old_processes()
participant Tailer as log tailer/watch
participant Kantra as Kantra test
Runner->>Shell: start job (JAVA=21, JVM_MAX_MEM=3072m)
Shell->>Podman: podman cp (maven.default.index, fernflower.jar, jdtls) -> workspace
Shell->>FS: mv ./default/generated -> ~/.config/.kantra/rulesets
Shell->>Cleaner: start cleanup_old_processes(...) [background]
Shell->>Tailer: start continuous_log_tailer / watch_and_tail_logs [background]
Shell->>Kantra: RUN_LOCAL=1 ./kantra test ./rulesets/...-removals.test.yaml
Kantra->>FS: write output.log, output.md, static-report
Shell->>Shell: parse output.md -> set GITHUB_OUTPUT (Rules Summary, Test Cases Summary)
Cleaner-->>Shell: periodic termination of older matching java/jdtls processes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
pranavgaikwad
force-pushed
the
bug/useExactly
branch
3 times, most recently
from
463f4d6 to
f4554f0
Compare
pranavgaikwad
force-pushed
the
bug/useExactly
branch
8 times, most recently
from
c6659e9 to
ca11a43
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/local-ci.yaml (1)
59-61: Background cleanup process is never terminated.
CLEANUP_PIDis captured but never used. The background process will continue running until the job terminates. While GitHub Actions cleans up processes after job completion, it's good practice to explicitly terminate background processes, especially before script exit.🔎 Suggested fix to terminate the cleanup process
Add a trap or explicit kill at the end of the script block (before exit statements):
if [[ ${some_test_failed} -eq 1 ]] || [[ ${some_tcs_failed} -eq 1 ]]; then + kill $CLEANUP_PID 2>/dev/null || true exit 1 else + kill $CLEANUP_PID 2>/dev/null || true exit 0 fiOr use a trap at the beginning of the cleanup block:
trap "kill $CLEANUP_PID 2>/dev/null || true" EXIT
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (238)
default/generated/azure/tests/01-azure-aws-config.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/02-azure-file-system.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/03-azure-java-version.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/04-azure-logging.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/05-azure-os-specific.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/06-azure-password.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/07-eap-to-azure-appservice-certificates.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/08-eap-to-azure-appservice-datasource-driver.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/09-eap-to-azure-appservice-environment-variables.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/10-eap-to-azure-appservice-pom.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/11-spring-boot-to-azure-cache.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/12-spring-boot-to-azure-config-server.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/13-spring-boot-to-azure-database.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/14-spring-boot-to-azure-eureka.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/15-spring-boot-to-azure-feign.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/16-spring-boot-to-azure-identity-provider.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/17-spring-boot-to-azure-java-fx.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/18-spring-boot-to-azure-jks.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/19-spring-boot-to-azure-jms-broker.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/20-spring-boot-to-azure-mq-config.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/21-spring-boot-to-azure-port.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/22-spring-boot-to-azure-schedule-job.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/23-spring-boot-to-azure-static-content.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/24-spring-boot-to-azure-swing.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/25-spring-boot-to-azure-system-config.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/26-spring-boot-to-azure-version.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/27-spring-boot-to-azure-zipkin.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/28-spring-cloud-to-azure-version.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/29-tomcat-to-azure-external-resources.windup.test.yamlis excluded by!**/generated/**default/generated/azure/tests/postgres-to-aks-workload-identity.test.yamlis excluded by!**/generated/**default/generated/camel3/34-java-generic-information.windup.yamlis excluded by!**/generated/**default/generated/camel3/tests/30-component-changes.groovy.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/31-component-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/32-java-dsl-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/33-java-generic-information.groovy.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/34-java-generic-information.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/35-java-multiple-camelcontexts-per-application-not-supported.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/36-xml-31-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/37-xml-310-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/38-xml-311-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/39-xml-312-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/40-xml-313-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/41-xml-314-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/42-xml-315-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/43-xml-316-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/44-xml-317-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/45-xml-318-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/46-xml-319-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/47-xml-32-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/48-xml-320-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/49-xml-321-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/50-xml-33-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/51-xml-34-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/52-xml-35-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/53-xml-36-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/54-xml-37-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/55-xml-38-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/56-xml-39-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/57-xml-dsl-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/58-xml-java-versions.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/59-xml-moved-components.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/60-xml-removed-components.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/61-xml-renamed-components.windup.test.yamlis excluded by!**/generated/**default/generated/camel3/tests/data/34-java-generic-information/src/main/java/com/example/apps/PojoAnnotations48Test.javais excluded by!**/generated/**default/generated/camel4/tests/62-xml-40-changes.windup.test.yamlis excluded by!**/generated/**default/generated/camel4/tests/63-xml-41-changes.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/65-java-corba.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/66-java-rmi.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/67-java-rpc.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/68-jca.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/69-jni-native-code.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/70-local-storage.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/71-localhost.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/73-mail.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/74-session.windup.test.yamlis excluded by!**/generated/**default/generated/cloud-readiness/tests/75-socket-communication.windup.test.yamlis excluded by!**/generated/**default/generated/droolsjbpm/tests/76-kie-api.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/77-commonj.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/79-java-ee-jaxrpc.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/80-seam-java.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/81-seam-ui.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/82-xml-webservices.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/83-eap4-xml-config.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/84-jboss-eap5-java.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/85-jboss-eap5-xml.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/86-resteasy.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/88-jotm.rhamt.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/91-jsp.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/92-log4j.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/95-environment-dependent.windup.test.yamlis excluded by!**/generated/**default/generated/eap6/tests/96-generic-catchall.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/137-weblogic.windup.yamlis excluded by!**/generated/**default/generated/eap7/140-websphere-jms.windup.yamlis excluded by!**/generated/**default/generated/eap7/tests/100-base64.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/102-jboss-eap5-7-java.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/103-jboss-eap5-7-xml.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/104-jboss-eap5and6to7-java.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/105-jboss-eap5and6to7-xml.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/106-resteasy.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/108-eap6.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/109-hibernate4-xml.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/110-hibernate4.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/111-hsearch.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/111b-hsearch.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/112-jax-ws.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/113-resteasy.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/114-ws-security.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/115-hibernate50-51.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/116-hibernate51-53.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/117-picketlink25.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/118-resteasy30-36.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/119-deprecated-singletonpolicy.rhamt.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/120-maven-artemis-jms-client.rhamt.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/121-maven-javax-to-jakarta.rhamt.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/122-maven-jboss-rmi-api_1.0_spec.rhamt.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/123-move-to-microprofile-rest-client-1.3.rhamt.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/124-singleton-sessionbean.rhamt.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/125-microprofile_removed_from_eap.mta.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/126-elytron.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/129-weblogic-catchall.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/130-weblogic-ejb.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/132-weblogic-jms.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/133-weblogic-services.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/134-weblogic-webapp.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/135-weblogic-webservices.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/136-weblogic-xml-descriptors.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/137-weblogic.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/138-websphere-catchall.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/140-websphere-jms.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/141-websphere-mq.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/142-websphere-mqe.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/143-websphere-other.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/144-websphere-xml.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/98-jboss-eap4and5to6and7-java.windup.test.yamlis excluded by!**/generated/**default/generated/eap7/tests/99-jboss-eap4and5to6and7-xml.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/153-hibernate-search.windup.yamlis excluded by!**/generated/**default/generated/eap8/154-hibernate6.windup.yamlis excluded by!**/generated/**default/generated/eap8/tests/145-eap8-faces.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/146-eap8-resteasy.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/147-eap8-xml-binding.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/148-eap8.ejb.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/149-deprecated-initialcontextfactory-is-removed.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/150-empty-beans-xml.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/151-hibernate-6.2.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/152-hibernate-search-6.1.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/153-hibernate-search.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/154-hibernate6.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/155-jakarta-cdi.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/156-jakarta-el.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/157-jakarta-faces.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/158-jakarta-json-binding.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/159-jakarta-soap.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/160-jakarta-ws-rs.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/161-javaee-to-jakarta-namespaces.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/162-javax-to-jakarta-bootstrapping-files.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/163-javax-to-jakarta-dependencies.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/164-javax-to-jakarta-package.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/165-javax-to-jakarta-properties.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/166-javax-to-jakarta-servlet.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/167-jboss-dependencies.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/168-keycloak-openid.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/169-legacy-vault.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/170-log4j-removed.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/170b-log4j-removed-no-config.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/171-login-modules.windup.test.yamlis excluded by!**/generated/**default/generated/eap8/tests/172-picketlink.windup.test.yamlis excluded by!**/generated/**default/generated/eapxp/tests/173-eapxp_bootable_jar_maven_plugin_configuration_changes.mta.test.yamlis excluded by!**/generated/**default/generated/eapxp/tests/174-eapxp_microprofile_metrics_not_supported.mta.test.yamlis excluded by!**/generated/**default/generated/eapxp/tests/175-eapxp_microprofile_opentracing_not_supported.mta.test.yamlis excluded by!**/generated/**default/generated/eapxp/tests/176-remove_thorntail_yaml_configuration_files.mta.test.yamlis excluded by!**/generated/**default/generated/eapxp/tests/177-replace_thorntail_boms.mta.test.yamlis excluded by!**/generated/**default/generated/eapxp/tests/178-replace_thorntail_fractions.mta.test.yamlis excluded by!**/generated/**default/generated/eapxp/tests/179-replace_thorntail_maven_plugin.mta.test.yamlis excluded by!**/generated/**default/generated/fuse-service-works/185-soa-p-5.windup.yamlis excluded by!**/generated/**default/generated/fuse-service-works/tests/185-soa-p-5.windup.test.yamlis excluded by!**/generated/**default/generated/fuse/tests/182-sonic-catchall.windup.test.yamlis excluded by!**/generated/**default/generated/fuse/tests/183-sonic-esb.windup.test.yamlis excluded by!**/generated/**default/generated/hibernate/tests/187-hibernate-xml.windup.test.yamlis excluded by!**/generated/**default/generated/hibernate/tests/188-hibernate.windup.test.yamlis excluded by!**/generated/**default/generated/jakarta-ee9/tests/189-spring-components.windup.test.yamlis excluded by!**/generated/**default/generated/jws6/tests/190-tomcat-dependencies.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk11/191-java-removals.windup.yamlis excluded by!**/generated/**default/generated/openjdk11/tests/191-java-removals.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk11/tests/192-removed-javaee-modules.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk17/tests/193-applet-api-deprecation.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk17/tests/194-lombok.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk17/tests/195-removed-classes.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk17/tests/196-removed-packages.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk17/tests/197-security-manager-deprecation.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk21/tests/198-deprecation-openjdk18.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk21/tests/199-deprecation-openjdk19.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk21/tests/200-deprecation-openjdk20.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk21/tests/201-deprecation-openjdk21.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk21/tests/202-dynamic-agents.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk21/tests/203-finalization-deprecation.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk21/tests/204-removed-apis.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk21/tests/205-utf-8-by-default.windup.test.yamlis excluded by!**/generated/**default/generated/openjdk7/tests/206-oracle2openjdk.rhamt.test.yamlis excluded by!**/generated/**default/generated/openliberty/tests/207-liberty-java-unavailable-technologies.windup.test.yamlis excluded by!**/generated/**default/generated/openliberty/tests/208-liberty-websphere-unavailable-technologies.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/200-ee-to-quarkus.windup.yamlis excluded by!**/generated/**default/generated/quarkus/tests/210-cdi-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/211-dependency-removal-for-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/212-jakarta-cdi-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/213-jakarta-faces-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/214-jakarta-jaxrs-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/215-javaee-faces-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/216-javaee-pom-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/217-jaxrs-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/218-jms-to-reactive-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/219-springboot-actuator-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/220-springboot-annotations-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/221-springboot-cache-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/222-springboot-cloud-config-client-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/223-springboot-data-jpa-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/224-springboot-devtools-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/225-springboot-di-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/226-springboot-generic-catchall.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/227-springboot-integration-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/228-springboot-jmx-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/229-springboot-metrics-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/230-springboot-parent-pom-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/231-springboot-plugins-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/232-springboot-properties-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/233-springboot-scheduled-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/234-springboot-security-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/235-springboot-shell-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/236-springboot-web-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/237-springboot-webmvc-to-quarkus.windup.test.yamlis excluded by!**/generated/**default/generated/quarkus/tests/data/216-javaee-pom-to-quarkus/pom.xmlis excluded by!**/generated/**default/generated/rhr/tests/238-springboot-associated-artifacts.rhamt.test.yamlis excluded by!**/generated/**default/generated/rhr/tests/239-springboot-rhr.rhamt.test.yamlis excluded by!**/generated/**default/generated/spring-boot/tests/spring-boot-2.x-to-3.0-removals.test.yamlis excluded by!**/generated/**default/generated/spring-framework/.spring-framework-5.x-to-6.0-security.yaml.swpis excluded by!**/generated/**default/generated/spring-framework/tests/spring-framework-5.x-to-6.0-data-access.test.yamlis excluded by!**/generated/**default/generated/spring-framework/tests/spring-framework-5.x-to-6.0-security-b.test.yamlis excluded by!**/generated/**default/generated/technology-usage/08-security-technology-usage.windup.yamlis excluded by!**/generated/**default/generated/technology-usage/277-observability.windup.yamlis excluded by!**/generated/**
📒 Files selected for processing (1)
.github/workflows/local-ci.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (4)
.github/workflows/local-ci.yaml (4)
19-19: Java 21 upgrade looks good.Java 21 is the current LTS version. Ensure that any rules or tests in the repository are compatible with Java 21 features and bytecode.
26-56: Cleanup workaround is documented and reasonable as a temporary fix.The logic correctly identifies and kills older processes while keeping the newest. A few observations:
- Using
kill -9(SIGKILL) is aggressive but appropriate here since graceful shutdown isn't needed for leaked processes.- Consider adding a TODO or issue reference to track removal of this workaround once the upstream memory leak is fixed.
71-73: Path references are consistent with the directory restructuring.The test command now correctly references
./rulesetswhich matches the destination of themvcommand on line 64. Thestatic-reportdirectory creation appears to be a new requirement for the kantra tool.
63-65: The./default/generateddirectory exists in the repository and contains valid migration rule sets. Themvcommand will execute successfully.
pranavgaikwad
force-pushed
the
bug/useExactly
branch
from
744d574 to
a591d1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/local-ci.yaml (2)
53-53: Consider graceful shutdown before SIGKILL.Using
kill -9(SIGKILL) immediately terminates processes without allowing cleanup. Consider sending SIGTERM first and only using SIGKILL if processes don't respond.🔎 Proposed graceful shutdown approach
-echo "$to_kill" | xargs -r kill -9 2>/dev/null || true +# Try graceful shutdown first +echo "$to_kill" | xargs -r kill 2>/dev/null || true +sleep 2 +# Force kill if still running +for pid in $to_kill; do + if kill -0 "$pid" 2>/dev/null; then + kill -9 "$pid" 2>/dev/null || true + fi +done
61-104: Consider reducing monitor verbosity or making it conditional.The
monitor_systemfunction logs extensive output every 60 seconds, which could significantly clutter CI logs. Since this is a temporary debugging workaround for memory leaks (per line 29), consider:
- Reducing the logging frequency
- Making verbose monitoring conditional (e.g., only on failure or via an env variable)
- Logging to a separate file and uploading as an artifact
Example conditional approach:
+# Start background system monitor only if DEBUG is set -monitor_system 60 & -MONITOR_PID=$! +if [[ "${DEBUG_MONITOR:-0}" == "1" ]]; then + monitor_system 60 & + MONITOR_PID=$! + echo "Started background monitor (PID: $MONITOR_PID)" +fi
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/local-ci.yaml
🔇 Additional comments (2)
.github/workflows/local-ci.yaml (2)
28-114: Verify alignment between PR objectives and actual changes.The PR description states this PR changes test assertions from "atLeast" to "exactly", but this workflow file shows infrastructure changes (Java version upgrade, background monitoring, path changes). Ensure these CI changes are intentional and documented.
Please confirm:
- Are the CI workflow changes in scope for this PR?
- Should the test assertion changes be in different files (e.g., test source files)?
- Is the path change from
./default/generatedto~/.config/.kantra/rulesetsrelated to the test assertion changes?Also applies to: 116-126
19-19: Remove the Java 21 compatibility concern.This repository is a ruleset collection for static code analysis, not a Java application. The Java version in the workflow is only used to run the
kantratool, which is extracted from a container image. There is no Java compilation, dependency resolution, or application code affected by this change. The kantra container handles its own tool compatibility regardless of the host JVM version.Likely an incorrect or invalid review comment.
| #### Block_Start #### | ||
| # This block cleans up old java processes to avoid leaking memory, this will be removed when we fix the memory leak in the java provider. | ||
| # Function to cleanup old processes matching a pattern, keeping only the newest | ||
| # Uses pgrep -f for pattern matching (matches against full command line) | ||
| # Example patterns: "java", "jdtls", "org.eclipse" | ||
| cleanup_old_processes() { | ||
| local pattern="$1" | ||
| local interval="${2:-30}" | ||
|
|
||
| while true; do | ||
| # Get PIDs of processes matching the pattern (pgrep -f matches full command line) | ||
| pids=$(pgrep -f "$pattern" 2>/dev/null | grep -v $$ || true) | ||
| if [[ -n "$pids" ]]; then | ||
| # Debug: show matching processes with details | ||
| echo "[cleanup] Matching '$pattern' processes:" | ||
| echo "$pids" | xargs -I{} ps -p {} -o pid,etimes,rss,comm --no-headers 2>/dev/null || true | ||
|
|
||
| # Sort by elapsed time descending (oldest first has largest elapsed time) | ||
| sorted_pids=$(echo "$pids" | xargs -I{} ps -p {} -o pid=,etimes= 2>/dev/null | sort -k2 -rn | awk '{print $1}') | ||
| count=$(echo "$sorted_pids" | grep -c . 2>/dev/null || echo 0) | ||
|
|
||
| if [[ $count -gt 1 ]]; then | ||
| # Kill all but the last one (newest has smallest elapsed time) | ||
| to_kill=$(echo "$sorted_pids" | head -n -1) | ||
| echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 1)) oldest: $(echo $to_kill | tr '\n' ' ')" | ||
| echo "$to_kill" | xargs -r kill -9 2>/dev/null || true | ||
| fi | ||
| fi | ||
| sleep "$interval" | ||
| done | ||
| } | ||
|
|
||
| # Background system monitor to debug hangs | ||
| monitor_system() { | ||
| local interval="${1:-60}" | ||
| local iteration=0 | ||
|
|
||
| while true; do | ||
| iteration=$((iteration + 1)) | ||
| echo "" | ||
| echo "========== [MONITOR #$iteration @ $(date)] ==========" | ||
|
|
||
| # Memory usage | ||
| echo "--- MEMORY ---" | ||
| free -h | ||
|
|
||
| # Disk usage | ||
| echo "--- DISK ---" | ||
| df -h / /tmp 2>/dev/null || df -h | ||
|
|
||
| # Top 10 memory-consuming processes | ||
| echo "--- TOP 10 PROCESSES BY MEMORY ---" | ||
| ps aux --sort=-%mem | head -11 | ||
|
|
||
| # Count of java/jdt processes | ||
| echo "--- JAVA PROCESS COUNT ---" | ||
| echo "Total java processes: $(pgrep -f java 2>/dev/null | wc -l)" | ||
| echo "JDT-LS processes: $(pgrep -f 'org.eclipse.jdt.ls' 2>/dev/null | wc -l)" | ||
|
|
||
| # Check for OOM killer activity | ||
| echo "--- OOM KILLER CHECK ---" | ||
| dmesg 2>/dev/null | grep -i "out of memory\|oom\|killed process" | tail -5 || echo "No OOM events (or no access to dmesg)" | ||
|
|
||
| # Check if kantra is still running | ||
| echo "--- KANTRA STATUS ---" | ||
| if pgrep -f "kantra test" > /dev/null 2>&1; then | ||
| echo "kantra test is running (PID: $(pgrep -f 'kantra test'))" | ||
| else | ||
| echo "kantra test is NOT running" | ||
| fi | ||
|
|
||
| echo "========== [END MONITOR #$iteration] ==========" | ||
| echo "" | ||
|
|
||
| sleep "$interval" | ||
| done | ||
| } | ||
|
|
||
| # Start background cleanup for JDT Language Server processes started by the Java provider | ||
| cleanup_old_processes "org.eclipse.jdt.ls" 30 & | ||
| CLEANUP_PID=$! | ||
|
|
||
| # Start background system monitor (logs every 60 seconds) | ||
| monitor_system 60 & | ||
| MONITOR_PID=$! | ||
| echo "Started background monitor (PID: $MONITOR_PID) and cleanup (PID: $CLEANUP_PID)" | ||
| #### Block_End #### |
There was a problem hiding this comment.
Background processes are never terminated.
The cleanup and monitoring processes started on lines 107-113 run indefinitely and are never cleaned up when the script completes or fails. This leaves orphaned background processes.
🔎 Proposed fix to terminate background processes
Add a trap to ensure cleanup on script exit:
# Start background cleanup for JDT Language Server processes started by the Java provider
cleanup_old_processes "org.eclipse.jdt.ls" 30 &
CLEANUP_PID=$!
# Start background system monitor (logs every 60 seconds)
monitor_system 60 &
MONITOR_PID=$!
echo "Started background monitor (PID: $MONITOR_PID) and cleanup (PID: $CLEANUP_PID)"
+
+# Ensure background processes are terminated on exit
+trap "echo 'Terminating background processes...'; kill $CLEANUP_PID $MONITOR_PID 2>/dev/null || true" EXIT
#### Block_End ####🤖 Prompt for AI Agents
.github/workflows/local-ci.yaml around lines 28 to 114: the background
cleanup_old_processes and monitor_system are started in the background but never
terminated, leaving orphaned processes; add a trap handler near where you start
them that captures EXIT, INT, and TERM and stops both background jobs (kill the
PIDs stored in CLEANUP_PID and MONITOR_PID if set, wait for them to exit, and
ignore failures), and ensure the trap also runs on error (set -o errexit or use
trap on EXIT) so the background processes are always killed when the script
finishes or is interrupted.
There was a problem hiding this comment.
@coderabbitai Would that stop the runner from ever reaching completion? Shouldn't the step exit as soon as tests complete?
There was a problem hiding this comment.
@pranavgaikwad No, the trap won't prevent the runner from completing. Here's how it works:
-
The step will exit as soon as the tests complete - the trap on
EXITonly executes when the script is exiting (either viaexit 0orexit 1at the end of your test logic) -
The trap runs cleanup, then allows the exit - it executes the kill commands for the background processes and then immediately proceeds with the normal exit
-
Without the trap, you have a problem - the background processes run in infinite loops (
while true), so when the main script exits, they continue running indefinitely as orphaned processes in the GitHub Actions runner
The execution flow with the trap would be:
tests complete → exit 0/1 triggered → trap handler runs (kills background processes) → script exits → step completes
The trap adds only milliseconds of cleanup time. The benefit is that it prevents resource leaks from orphaned background processes that would otherwise continue consuming runner resources after your step completes.
| mkdir -p ~/.config/.kantra | ||
| mv ./default/generated ~/.config/.kantra/rulesets | ||
| cd ~/.config/.kantra |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add validation for source directory before moving.
The script assumes ./default/generated exists before attempting to move it. If this directory doesn't exist, the mv command will fail and the workflow will error.
🔎 Proposed fix to add path validation
mkdir -p ~/.config/.kantra
+if [ ! -d "./default/generated" ]; then
+ echo "Error: ./default/generated directory not found"
+ exit 1
+fi
mv ./default/generated ~/.config/.kantra/rulesets
cd ~/.config/.kantraOr, if the directory might not exist in some scenarios:
mkdir -p ~/.config/.kantra
+if [ -d "./default/generated" ]; then
+ mv ./default/generated ~/.config/.kantra/rulesets
+else
+ echo "Warning: ./default/generated not found, creating empty rulesets directory"
+ mkdir -p ~/.config/.kantra/rulesets
+fi
-mv ./default/generated ~/.config/.kantra/rulesets
cd ~/.config/.kantra📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| mkdir -p ~/.config/.kantra | |
| mv ./default/generated ~/.config/.kantra/rulesets | |
| cd ~/.config/.kantra | |
| mkdir -p ~/.config/.kantra | |
| if [ ! -d "./default/generated" ]; then | |
| echo "Error: ./default/generated directory not found" | |
| exit 1 | |
| fi | |
| mv ./default/generated ~/.config/.kantra/rulesets | |
| cd ~/.config/.kantra |
🤖 Prompt for AI Agents
In .github/workflows/local-ci.yaml around lines 116 to 118, the workflow
unconditionally runs mv ./default/generated ~/.config/.kantra/rulesets which
will fail if ./default/generated doesn't exist; update the script to validate
the source directory first (e.g., test -d ./default/generated) and only run mv
when it exists, otherwise create an empty rulesets dir or skip the move so the
workflow doesn't error; ensure the commands maintain the same working directory
behavior (cd ~/.config/.kantra) after the conditional.
There was a problem hiding this comment.
@coderabbitai bro, it does exist, we know.
There was a problem hiding this comment.
@pranavgaikwad, got it! I'll trust that ./default/generated exists in your workflow setup. Thanks for clarifying!
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
…account for accuracy fixes Shawn made Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
pranavgaikwad
force-pushed
the
bug/useExactly
branch
from
a591d1b to
6c25b49
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/local-ci.yaml (1)
141-147: Update artifact upload to target current test output location or remove the step.The artifact upload step targets
/tmp/rules-test-*, but tests have been restructured to run in~/.config/.kantrawith output written tooutput.log. Either update the artifact path to the new location or remove this step if the artifacts are no longer needed.
🧹 Nitpick comments (1)
.github/workflows/local-ci.yaml (1)
25-27: Redundant JVM_MAX_MEM declaration on line 115.The
JVM_MAX_MEMenvironment variable is set at the step level here, which makes it available throughout the entire step. The inline declarationJVM_MAX_MEM=4Gon line 115 is redundant.♻️ Simplify by removing the inline declaration
On line 115, remove the inline
JVM_MAX_MEM=4Gprefix:-JVM_MAX_MEM=4G RUN_LOCAL=1 ./kantra test ./rulesets --log-level 50 | tee output.log || true +RUN_LOCAL=1 ./kantra test ./rulesets --log-level 50 | tee output.log || true
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/local-ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Build tackle2-hub
🔇 Additional comments (3)
.github/workflows/local-ci.yaml (3)
110-113: LGTM! Asset extraction correctly relocated.The podman copy commands and static-report directory creation are correctly placed after the working directory change to
~/.config/.kantra, ensuring Kantra assets are extracted to the expected location.
115-115: Test path correctly updated to new ruleset location.The test command now correctly references
./rulesets, which resolves to~/.config/.kantra/rulesetsgiven the working directory change on line 107.
19-19: > Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @.github/workflows/local-ci.yaml:
- Around line 33-58: The cleanup_old_processes function has an off-by-one when
selecting processes to kill: sorted_pids is ordered oldest->newest and the code
uses head -n -2 which keeps only 2 newest instead of 3; change the to_kill
calculation (the line that sets to_kill) to use head -n -3 so it kills all but
the last 3 newest processes (keep existing guards like count > 3 and the kill
pipeline intact), referencing the cleanup_old_processes function and variables
sorted_pids and to_kill when making the edit.
- Around line 95-104: The background cleanup_old_processes is started in the
background and left running (CLEANUP_PID=$!) with no termination on step exit;
add an EXIT trap that kills and waits for CLEANUP_PID (and MONITOR_PID if used)
to ensure the background loop is terminated: register trap 'kill <pid> || true;
wait <pid> || true' (or equivalent) referencing CLEANUP_PID and MONITOR_PID and
set the trap before launching the background jobs so they are cleaned up when
the step exits.
- Around line 117-118: Restore the CI test invocation to run the full ruleset
suite by replacing the single-file test target string
"./rulesets/spring-boot/tests/spring-boot-2.x-to-3.0-removals.test.yaml" with
the original "./default/generated" in the "./kantra test ..." command, and
remove the redundant JVM_MAX_MEM=4G prefix from that run line (since JVM_MAX_MEM
is already set in the job environment) so the command uses the existing
environment variable rather than duplicating it.
🧹 Nitpick comments (1)
.github/workflows/local-ci.yaml (1)
25-27: Remove redundant JVM_MAX_MEM declaration.
JVM_MAX_MEMis set here at the step level and again on line 118 in the command invocation. The step-level declaration is sufficient; the command-line redeclaration is redundant.♻️ Simplify by removing the redundant declaration
Remove the duplicate on line 118:
-JVM_MAX_MEM=4G RUN_LOCAL=1 ./kantra test ./rulesets/spring-boot/tests/spring-boot-2.x-to-3.0-removals.test.yaml --log-level 50 | tee output.log || true +RUN_LOCAL=1 ./kantra test ./rulesets/spring-boot/tests/spring-boot-2.x-to-3.0-removals.test.yaml --log-level 50 | tee output.log || true
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/local-ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build tackle2-hub
- GitHub Check: test
🔇 Additional comments (3)
.github/workflows/local-ci.yaml (3)
109-115: LGTM!The Podman commands correctly copy Kantra assets to the current directory (
~/.config/.kantraafter thecdon line 108). The relative paths and directory creation forstatic-reportare appropriate.
145-151: Inconsistency: AI summary claims this step was removed.The AI-generated summary states "Removed the Upload matching folders step entirely", but this step is still present in the workflow. Additionally, with the test scope narrowed to a single test file (line 118), this artifact upload may no longer capture relevant test outputs, as the test results are now written to different locations (
~/.config/.kantra).Consider whether this step should be:
- Removed (as the AI summary suggests)
- Updated to upload artifacts from the new test location
- Kept as-is if it still serves a purpose
19-19: [Rewritten review comment]
[Classification tag]
| cleanup_old_processes() { | ||
| local pattern="$1" | ||
| local interval="${2:-30}" | ||
|
|
||
| while true; do | ||
| # Get PIDs of processes matching the pattern (pgrep -f matches full command line) | ||
| pids=$(pgrep -f "$pattern" 2>/dev/null | grep -v $$ || true) | ||
| if [[ -n "$pids" ]]; then | ||
| # Debug: show matching processes with details | ||
| echo "[cleanup] Matching '$pattern' processes:" | ||
| echo "$pids" | xargs -I{} ps -p {} -o pid,etimes,rss,comm --no-headers 2>/dev/null || true | ||
|
|
||
| # Sort by elapsed time descending (oldest first has largest elapsed time) | ||
| sorted_pids=$(echo "$pids" | xargs -I{} ps -p {} -o pid=,etimes= 2>/dev/null | sort -k2 -rn | awk '{print $1}') | ||
| count=$(echo "$sorted_pids" | grep -c . 2>/dev/null || echo 0) | ||
|
|
||
| if [[ $count -gt 3 ]]; then | ||
| # Kill all but the last 3 (newest have smallest elapsed time) | ||
| to_kill=$(echo "$sorted_pids" | head -n -2) | ||
| echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 3)) oldest: $(echo $to_kill | tr '\n' ' ')" | ||
| echo "$to_kill" | xargs -r kill -9 2>/dev/null || true | ||
| fi | ||
| fi | ||
| sleep "$interval" | ||
| done | ||
| } |
There was a problem hiding this comment.
Off-by-one error in process cleanup logic.
Line 51 uses head -n -2 which keeps only the 2 newest processes, not 3 as intended by the comment and log message. This means more processes will be retained than expected, reducing the effectiveness of the memory leak mitigation.
🐛 Fix the off-by-one error
if [[ $count -gt 3 ]]; then
- # Kill all but the last 3 (newest have smallest elapsed time)
- to_kill=$(echo "$sorted_pids" | head -n -2)
+ # Kill all but the last 3 (newest have smallest elapsed time)
+ to_kill=$(echo "$sorted_pids" | head -n -3)
echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 3)) oldest: $(echo $to_kill | tr '\n' ' ')"
echo "$to_kill" | xargs -r kill -9 2>/dev/null || true
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| cleanup_old_processes() { | |
| local pattern="$1" | |
| local interval="${2:-30}" | |
| while true; do | |
| # Get PIDs of processes matching the pattern (pgrep -f matches full command line) | |
| pids=$(pgrep -f "$pattern" 2>/dev/null | grep -v $$ || true) | |
| if [[ -n "$pids" ]]; then | |
| # Debug: show matching processes with details | |
| echo "[cleanup] Matching '$pattern' processes:" | |
| echo "$pids" | xargs -I{} ps -p {} -o pid,etimes,rss,comm --no-headers 2>/dev/null || true | |
| # Sort by elapsed time descending (oldest first has largest elapsed time) | |
| sorted_pids=$(echo "$pids" | xargs -I{} ps -p {} -o pid=,etimes= 2>/dev/null | sort -k2 -rn | awk '{print $1}') | |
| count=$(echo "$sorted_pids" | grep -c . 2>/dev/null || echo 0) | |
| if [[ $count -gt 3 ]]; then | |
| # Kill all but the last 3 (newest have smallest elapsed time) | |
| to_kill=$(echo "$sorted_pids" | head -n -2) | |
| echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 3)) oldest: $(echo $to_kill | tr '\n' ' ')" | |
| echo "$to_kill" | xargs -r kill -9 2>/dev/null || true | |
| fi | |
| fi | |
| sleep "$interval" | |
| done | |
| } | |
| cleanup_old_processes() { | |
| local pattern="$1" | |
| local interval="${2:-30}" | |
| while true; do | |
| # Get PIDs of processes matching the pattern (pgrep -f matches full command line) | |
| pids=$(pgrep -f "$pattern" 2>/dev/null | grep -v $$ || true) | |
| if [[ -n "$pids" ]]; then | |
| # Debug: show matching processes with details | |
| echo "[cleanup] Matching '$pattern' processes:" | |
| echo "$pids" | xargs -I{} ps -p {} -o pid,etimes,rss,comm --no-headers 2>/dev/null || true | |
| # Sort by elapsed time descending (oldest first has largest elapsed time) | |
| sorted_pids=$(echo "$pids" | xargs -I{} ps -p {} -o pid=,etimes= 2>/dev/null | sort -k2 -rn | awk '{print $1}') | |
| count=$(echo "$sorted_pids" | grep -c . 2>/dev/null || echo 0) | |
| if [[ $count -gt 3 ]]; then | |
| # Kill all but the last 3 (newest have smallest elapsed time) | |
| to_kill=$(echo "$sorted_pids" | head -n -3) | |
| echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 3)) oldest: $(echo $to_kill | tr '\n' ' ')" | |
| echo "$to_kill" | xargs -r kill -9 2>/dev/null || true | |
| fi | |
| fi | |
| sleep "$interval" | |
| done | |
| } |
🤖 Prompt for AI Agents
In @.github/workflows/local-ci.yaml around lines 33 - 58, The
cleanup_old_processes function has an off-by-one when selecting processes to
kill: sorted_pids is ordered oldest->newest and the code uses head -n -2 which
keeps only 2 newest instead of 3; change the to_kill calculation (the line that
sets to_kill) to use head -n -3 so it kills all but the last 3 newest processes
(keep existing guards like count > 3 and the kill pipeline intact), referencing
the cleanup_old_processes function and variables sorted_pids and to_kill when
making the edit.
| # Start background cleanup for JDT Language Server processes started by the Java provider | ||
| cleanup_old_processes "org.eclipse.jdt.ls" 30 & | ||
| CLEANUP_PID=$! | ||
|
|
||
| # Start background system monitor (logs every 60 seconds) | ||
| # COMMENT OUT FOR DEBUGGING | ||
| # monitor_system 60 & | ||
| # MONITOR_PID=$! | ||
| # echo "Started background monitor (PID: $MONITOR_PID) and cleanup (PID: $CLEANUP_PID)" | ||
| #### Block_End #### |
There was a problem hiding this comment.
Background cleanup process still not terminated on exit.
The cleanup_old_processes function runs an infinite loop and is started in the background, but there's no mechanism to terminate it when the workflow step completes. This leaves an orphaned process that continues running after the step exits.
As discussed in previous review comments, adding a trap handler would not prevent the step from completing—it would only add milliseconds of cleanup time while preventing resource leaks.
♻️ Add trap handler to clean up background processes
# Start background cleanup for JDT Language Server processes started by the Java provider
cleanup_old_processes "org.eclipse.jdt.ls" 30 &
CLEANUP_PID=$!
# Start background system monitor (logs every 60 seconds)
# COMMENT OUT FOR DEBUGGING
# monitor_system 60 &
# MONITOR_PID=$!
# echo "Started background monitor (PID: $MONITOR_PID) and cleanup (PID: $CLEANUP_PID)"
+
+# Terminate background processes on exit
+trap "echo 'Cleaning up background processes...'; kill $CLEANUP_PID 2>/dev/null || true; wait $CLEANUP_PID 2>/dev/null || true" EXIT
#### Block_End ####🤖 Prompt for AI Agents
In @.github/workflows/local-ci.yaml around lines 95 - 104, The background
cleanup_old_processes is started in the background and left running
(CLEANUP_PID=$!) with no termination on step exit; add an EXIT trap that kills
and waits for CLEANUP_PID (and MONITOR_PID if used) to ensure the background
loop is terminated: register trap 'kill <pid> || true; wait <pid> || true' (or
equivalent) referencing CLEANUP_PID and MONITOR_PID and set the trap before
launching the background jobs so they are cleaned up when the step exits.
| ( while true; do if [ -f ./rulesets/spring-boot/tests/data/removals/.metadata/.log ]; then tail -F ./rulesets/spring-boot/tests/data/removals/.metadata/.log; break; else sleep 2; fi; done ) & | ||
| JVM_MAX_MEM=4G RUN_LOCAL=1 ./kantra test ./rulesets/spring-boot/tests/spring-boot-2.x-to-3.0-removals.test.yaml --log-level 50 | tee output.log || true |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the full local-ci.yaml workflow to understand context
wc -l .github/workflows/local-ci.yaml
# Read the entire file to understand the workflow structure
cat -n .github/workflows/local-ci.yamlRepository: konveyor/rulesets
Length of output: 7576
🏁 Script executed:
# Check git history of the workflow file
git log --oneline -20 .github/workflows/local-ci.yaml
# Get the previous version of the relevant lines
echo "=== Checking previous version of lines around 117-118 ==="
git show HEAD~1:.github/workflows/local-ci.yaml | head -n 130 | tail -n 25Repository: konveyor/rulesets
Length of output: 211
🏁 Script executed:
# Check git status and branches
git status
git branch -a
# Check if there are other test files that should be tested
echo "=== Finding all test files in rulesets ==="
fd -e test.yaml . rulesets/ 2>/dev/null | head -20
# Check if there's a README or documentation about testing
echo ""
echo "=== Checking for test documentation ==="
fd -i "readme\|test\|contributing" . --max-depth 2 -type f 2>/dev/null | grep -E "(readme|test|contributing)" | head -10Repository: konveyor/rulesets
Length of output: 325
🏁 Script executed:
# Check the directory structure around the test file
echo "=== Checking rulesets directory structure ==="
ls -la rulesets/
# Look for the specific test file being run
echo ""
echo "=== Searching for test files containing 'removals' ==="
find rulesets/ -type f -name "*removals*" 2>/dev/null | head -20
# Check what actual test files exist
echo ""
echo "=== Looking for any files under spring-boot/tests ==="
ls -la rulesets/spring-boot/tests/ 2>/dev/null | head -20
# Compare with main branch
echo ""
echo "=== Checking main branch workflow ==="
git show origin/main:.github/workflows/local-ci.yaml | grep -A5 -B5 "kantra test" | head -20Repository: konveyor/rulesets
Length of output: 1193
Revert to testing all rulesets instead of a single test file.
The workflow has been changed from testing ./default/generated (all rulesets) to testing only ./rulesets/spring-boot/tests/spring-boot-2.x-to-3.0-removals.test.yaml. While the commit message "debug failing spring boot test" indicates this narrowing is intentional for debugging, this significantly reduces CI coverage and should not be merged. Restore the original test command to ./kantra test ./default/generated to maintain full test coverage.
Additionally, JVM_MAX_MEM=4G is set twice: once in the environment variables (line 26) and again in the run command (line 118). Remove the redundant setting from line 118.
🤖 Prompt for AI Agents
In @.github/workflows/local-ci.yaml around lines 117 - 118, Restore the CI test
invocation to run the full ruleset suite by replacing the single-file test
target string
"./rulesets/spring-boot/tests/spring-boot-2.x-to-3.0-removals.test.yaml" with
the original "./default/generated" in the "./kantra test ..." command, and
remove the redundant JVM_MAX_MEM=4G prefix from that run line (since JVM_MAX_MEM
is already set in the job environment) so the command uses the existing
environment variable rather than duplicating it.
7059121 to
a25f0fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/local-ci.yaml:
- Line 115: The backgrounded log tailing loop starting at the `tail -F
./rulesets/spring-boot/tests/data/removals/.metadata/.log` invocation must have
its PID captured and cleaned up in the existing trap handler: start the loop in
background, save its PID to a variable (e.g., TAIL_PID or LOG_TAIL_PID using
`$!`), and update the trap/cleanup routine (the same trap added per previous
comment) to check for and kill that PID if still running (and wait for it) so
the tail process cannot become orphaned after the step completes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/local-ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build tackle2-hub
- GitHub Check: test
🔇 Additional comments (2)
.github/workflows/local-ci.yaml (2)
143-149: Artifact upload may not capture expected test results with current single-test configuration.The artifact upload step references
/tmp/rules-test-*, which should work with the absolute path. However, with the workflow currently testing only a single test file (line 116), this pattern may not match the expected artifacts or may only capture results from one test instead of the full suite.Once the full test suite execution is restored, verify that this artifact pattern still captures all relevant test outputs.
19-19: No Java 21 compatibility issues found.The test data applications target Java 8–11, which are independent of the JVM version running Kantra. As a static analysis rules repository rather than a compiled Java application, this repository has no build configuration constraints against Java 21. Java 21 is a stable LTS release and suitable for running the analysis toolchain.
pranavgaikwad
force-pushed
the
bug/useExactly
branch
from
a25f0fe to
e2828ce
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @.github/workflows/local-ci.yaml:
- Around line 122-128: The nested backgrounded `tail -f` processes in
tail_test_logs are orphaned by only tracking the outer subshell; change the
implementation to explicitly track and clean up child tails: when you start each
tail (the commands starting with `tail -f "${dir}analysis.log"` and `tail -f
"${metadata_dir}/.log"`), capture their PIDs into a global array (e.g.,
TAIL_PIDS+=( $! )) instead of nesting backgrounding, and on exit/cleanup iterate
over TAIL_PIDS to kill them; alternatively, start the whole tailing in its own
process group (setsid or set -m and use negative PID to kill the group) and
ensure LOG_TAILER_PID refers to that group so killing it terminates all children
— update the code around the tail invocations and the LOG_TAILER_PID/cleanup
logic accordingly.
🧹 Nitpick comments (1)
.github/workflows/local-ci.yaml (1)
58-91: Consider removing or conditionally including the unusedmonitor_systemfunction.This function is defined but its invocation is commented out. If this is temporary debugging code, consider removing it before merge or guarding it behind an environment variable to reduce workflow size.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/local-ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: Build tackle2-hub
🔇 Additional comments (4)
.github/workflows/local-ci.yaml (4)
16-21: Java 21 upgrade looks good.Java 21 is the current LTS version and provides improved performance and language features. Ensure downstream components (Kantra, analyzers) are compatible with Java 21.
165-174: Resource setup looks correct.The workflow correctly sets up the Kantra containerless environment by copying required files to
~/.config/.kantra. Thepodman cpcommands now target the current working directory aftercd ~/.config/.kantra.
178-201: Output parsing and exit logic looks correct.The parsing of test results from
output.logcorrectly extracts pass rates and determines the exit code based on whether all tests passed.
203-209: Artifact upload configuration is appropriate.Uploading test directories with
if: always()ensures debugging artifacts are available even when tests fail. High compression is a good choice for CI storage efficiency.
pranavgaikwad
force-pushed
the
bug/useExactly
branch
2 times, most recently
from
822bc74 to
3a8a1eb
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.github/workflows/local-ci.yaml:
- Line 203: The workflow step currently hardcodes the test target to a single
ruleset file (the RUN_LOCAL ./kantra test invocation) which limits CI coverage;
change that command to run the full ruleset suite by replacing the single-file
argument with the rulesets glob (i.e., run the kantra test against the entire
rulesets collection rather than spring-boot-2.x-to-3.0-removals.test.yaml),
remove the temporary single-file override, and ensure the workflow restores the
original invocation so the full test suite executes before merging.
- Around line 47-51: The cleanup logic has an off-by-one: when computing to_kill
from sorted_pids it uses "head -n -2" which leaves 2 newest PIDs instead of the
intended 3, so change that invocation to "head -n -3" (variable names:
sorted_pids, to_kill, count, pattern) so you kill all but the last 3 processes
and ensure the log/kill arithmetic using $((count - 3)) remains correct.
- Around line 156-189: The background watcher watch_and_tail_logs spawns
continuous_log_tailer processes but never records their PIDs, so child tailers
can be orphaned when LOG_TAILER_PID is killed; modify watch_and_tail_logs to
either (a) launch itself in a dedicated process group and start children so they
inherit the group (use setsid or set -m / setpgid semantics) and ensure the
cleanup trap kills the whole group, or (b) append each background PID from
continuous_log_tailer to a global array (e.g., TAILER_PIDS) when starting them
and update the trap/cleanup logic to iterate over TAILER_PIDS to kill/cleanup
all child PIDs before exiting, making sure to reference the functions/variables
continuous_log_tailer, watch_and_tail_logs, LOG_TAILER_PID, and the new
TAILER_PIDS in your changes.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/local-ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build tackle2-hub
- GitHub Check: test
🔇 Additional comments (3)
.github/workflows/local-ci.yaml (3)
192-201: Resource relocation paths updated correctly.The relocation of generated rulesets to
~/.config/.kantra/rulesetsand the updatedpodman cpcommands align with the new directory structure. The changes are consistent and maintain the necessary resources for Kantra execution.
230-236: Inconsistency with AI summary: artifact upload step is not removed.The AI summary states "Removal of the Upload matching folders step (artifact upload) from the workflow," but this step is still present in the code (lines 230-236). The step remains unchanged from the previous version.
19-19: This concern is not applicable to this repository.The review assumes this is a Maven source project with dependencies to verify. However, konveyor/rulesets is a ruleset configuration repository with no Maven dependencies (no pom.xml files). Java 21 is used only to execute pre-built tools (Kantra, jdtls) downloaded from container registries, not to compile project code. Kantra compatibility with Java 21 would be an upstream concern for the konveyor/kantra project, not this repository.
Likely an incorrect or invalid review comment.
| if [[ $count -gt 3 ]]; then | ||
| # Kill all but the last 3 (newest have smallest elapsed time) | ||
| to_kill=$(echo "$sorted_pids" | head -n -2) | ||
| echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 3)) oldest: $(echo $to_kill | tr '\n' ' ')" | ||
| echo "$to_kill" | xargs -r kill -9 2>/dev/null || true |
There was a problem hiding this comment.
Off-by-one error: head -n -2 keeps only 2 processes, not 3.
Line 49 uses head -n -2 which keeps only the 2 newest processes, not 3 as indicated by the comment and log message. This means one additional process will be killed than intended, reducing the effectiveness of the memory leak mitigation.
🐛 Fix the off-by-one error
if [[ $count -gt 3 ]]; then
# Kill all but the last 3 (newest have smallest elapsed time)
- to_kill=$(echo "$sorted_pids" | head -n -2)
+ to_kill=$(echo "$sorted_pids" | head -n -3)
echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 3)) oldest: $(echo $to_kill | tr '\n' ' ')"
echo "$to_kill" | xargs -r kill -9 2>/dev/null || true
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ $count -gt 3 ]]; then | |
| # Kill all but the last 3 (newest have smallest elapsed time) | |
| to_kill=$(echo "$sorted_pids" | head -n -2) | |
| echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 3)) oldest: $(echo $to_kill | tr '\n' ' ')" | |
| echo "$to_kill" | xargs -r kill -9 2>/dev/null || true | |
| if [[ $count -gt 3 ]]; then | |
| # Kill all but the last 3 (newest have smallest elapsed time) | |
| to_kill=$(echo "$sorted_pids" | head -n -3) | |
| echo "[cleanup] Found $count processes matching '$pattern', killing $((count - 3)) oldest: $(echo $to_kill | tr '\n' ' ')" | |
| echo "$to_kill" | xargs -r kill -9 2>/dev/null || true |
🤖 Prompt for AI Agents
In @.github/workflows/local-ci.yaml around lines 47 - 51, The cleanup logic has
an off-by-one: when computing to_kill from sorted_pids it uses "head -n -2"
which leaves 2 newest PIDs instead of the intended 3, so change that invocation
to "head -n -3" (variable names: sorted_pids, to_kill, count, pattern) so you
kill all but the last 3 processes and ensure the log/kill arithmetic using
$((count - 3)) remains correct.
| # Start background cleanup for JDT Language Server processes started by the Java provider | ||
| cleanup_old_processes "org.eclipse.jdt.ls" 30 & | ||
| CLEANUP_PID=$! | ||
|
|
||
| # Start background system monitor (logs every 60 seconds) | ||
| # COMMENT OUT FOR DEBUGGING | ||
| # monitor_system 60 & | ||
| # MONITOR_PID=$! | ||
| # echo "Started background monitor (PID: $MONITOR_PID) and cleanup (PID: $CLEANUP_PID)" | ||
|
|
There was a problem hiding this comment.
Background cleanup process lacks termination handler.
The cleanup_old_processes function runs an infinite loop and is started in the background (line 94), but there's no trap handler to terminate it when the workflow step completes. This leaves an orphaned process consuming runner resources.
As clarified in previous discussion: a trap on EXIT executes when the script exits and only adds milliseconds of cleanup time—it does not prevent the step from completing.
♻️ Add trap handler to terminate background cleanup
# Start background cleanup for JDT Language Server processes started by the Java provider
cleanup_old_processes "org.eclipse.jdt.ls" 30 &
CLEANUP_PID=$!
# Start background system monitor (logs every 60 seconds)
# COMMENT OUT FOR DEBUGGING
# monitor_system 60 &
# MONITOR_PID=$!
# echo "Started background monitor (PID: $MONITOR_PID) and cleanup (PID: $CLEANUP_PID)"
+
+# Terminate background processes on exit
+trap "echo 'Cleaning up background processes...'; kill $CLEANUP_PID 2>/dev/null || true; wait $CLEANUP_PID 2>/dev/null || true" EXIT📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Start background cleanup for JDT Language Server processes started by the Java provider | |
| cleanup_old_processes "org.eclipse.jdt.ls" 30 & | |
| CLEANUP_PID=$! | |
| # Start background system monitor (logs every 60 seconds) | |
| # COMMENT OUT FOR DEBUGGING | |
| # monitor_system 60 & | |
| # MONITOR_PID=$! | |
| # echo "Started background monitor (PID: $MONITOR_PID) and cleanup (PID: $CLEANUP_PID)" | |
| # Start background cleanup for JDT Language Server processes started by the Java provider | |
| cleanup_old_processes "org.eclipse.jdt.ls" 30 & | |
| CLEANUP_PID=$! | |
| # Start background system monitor (logs every 60 seconds) | |
| # COMMENT OUT FOR DEBUGGING | |
| # monitor_system 60 & | |
| # MONITOR_PID=$! | |
| # echo "Started background monitor (PID: $MONITOR_PID) and cleanup (PID: $CLEANUP_PID)" | |
| # Terminate background processes on exit | |
| trap "echo 'Cleaning up background processes...'; kill $CLEANUP_PID 2>/dev/null || true; wait $CLEANUP_PID 2>/dev/null || true" EXIT |
pranavgaikwad
force-pushed
the
bug/useExactly
branch
from
3a8a1eb to
9e5a25e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @.github/workflows/local-ci.yaml:
- Around line 83-85: Start a shell EXIT trap immediately after spawning the
background loops so they won’t be orphaned: after launching
cleanup_old_processes "org.eclipse.jdt.ls" 30 & (which sets CLEANUP_PID=$!) and
after launching watch_and_tail_logs ... & (capture WATCH_PID=$!), register a
trap 'trap "kill -TERM $CLEANUP_PID $WATCH_PID 2>/dev/null || true" EXIT' (or
similar) before the script reaches the block end; this ensures both background
PIDs are killed on script exit and the trap is set right after starting those
processes.
- Around line 140-173: The background watcher spawns untracked child tailers
(continuous_log_tailer) and they can become orphans; start the watcher in its
own process group and track that group so kills propagate: launch the watcher
with setsid (e.g., use setsid watch_and_tail_logs &), capture LOG_TAILER_PID=$!
and the process group id (e.g., LOG_TAILER_PGID=$(ps -o pgid= $LOG_TAILER_PID |
tr -d ' ')), and ensure any shutdown/cleanup uses kill -- -$LOG_TAILER_PGID to
terminate the entire group (references: watch_and_tail_logs,
continuous_log_tailer, LOG_TAILER_PID).
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/local-ci.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Build tackle2-hub
- GitHub Check: test
🔇 Additional comments (4)
.github/workflows/local-ci.yaml (4)
179-185: LGTM: Container artifact extraction updated correctly.The podman commands properly extract the necessary artifacts (kantra binary, maven index, fernflower, jdtls) to the new directory structure at
~/.config/.kantra/. The static-report directory creation is also appropriate.
214-220: Inconsistency with AI summary - artifact upload step still present.The AI-generated summary states "Removal of the Upload matching folders step (artifact upload)" but this step is still present in the workflow (lines 214-220). The step uploads test folders for debugging, which is useful for troubleshooting failures.
Since the code shows the step is present, no action is needed unless the removal was intended but not completed.
87-138: LGTM: Polling-based log tailer handles edge cases correctly.The
continuous_log_tailerfunction properly handles file rotation (via inode tracking), truncation, and growth. The status interval prevents silent hangs, and the byte-offset approach withtail -cis efficient for reading new content.
19-19: This review comment is not applicable to this repository. The konveyor/rulesets repository is a ruleset definitions repository, not a Java application project. Kantra runs as a containerized tool (quay.io/konveyor/kantra:latest) and is platform-agnostic; the host Java version does not affect Kantra execution. Maven 3.9.0 (specified in the workflow) fully supports Java 21. The upgrade from Java 17 to 21 is safe and poses no compatibility concerns for this codebase.Likely an incorrect or invalid review comment.
| # Start background cleanup for JDT Language Server processes started by the Java provider | ||
| cleanup_old_processes "org.eclipse.jdt.ls" 30 & | ||
| CLEANUP_PID=$! |
There was a problem hiding this comment.
Background processes will orphan without cleanup trap.
Both cleanup_old_processes (line 84) and watch_and_tail_logs (line 171) run infinite loops but lack termination handlers. When the workflow step exits, these processes continue consuming runner resources.
As clarified in previous discussion: a trap on EXIT executes during script exit and adds only milliseconds of overhead—it does not prevent the step from completing normally.
🔧 Add trap handler to terminate background processes
# Start background cleanup for JDT Language Server processes started by the Java provider
cleanup_old_processes "org.eclipse.jdt.ls" 30 &
CLEANUP_PID=$!
+
+# Start background log watcher
+watch_and_tail_logs &
+LOG_TAILER_PID=$!
+echo "Started log tailer (PID: $LOG_TAILER_PID)"
+
+# Terminate background processes on exit
+trap "echo 'Cleaning up background processes...'; kill $CLEANUP_PID $LOG_TAILER_PID 2>/dev/null || true; wait $CLEANUP_PID $LOG_TAILER_PID 2>/dev/null || true" EXIT
+#### Block_End ####Note: Move the trap registration to immediately after starting both background processes, before #### Block_End ####.
Also applies to: 170-173
🤖 Prompt for AI Agents
In @.github/workflows/local-ci.yaml around lines 83 - 85, Start a shell EXIT
trap immediately after spawning the background loops so they won’t be orphaned:
after launching cleanup_old_processes "org.eclipse.jdt.ls" 30 & (which sets
CLEANUP_PID=$!) and after launching watch_and_tail_logs ... & (capture
WATCH_PID=$!), register a trap 'trap "kill -TERM $CLEANUP_PID $WATCH_PID
2>/dev/null || true" EXIT' (or similar) before the script reaches the block end;
this ensures both background PIDs are killed on script exit and the trap is set
right after starting those processes.
e4e150c to
dd4e424
Compare
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
Signed-off-by: Pranav Gaikwad <pgaikwad@redhat.com>
pranavgaikwad
force-pushed
the
bug/useExactly
branch
from
dd4e424 to
4a2207f
Compare
|
@shawn-hurley @jmle I fixed all issues with the tests now. There are a few (< 1%) tests that intermittently fail. But I have set the threshold to 99% for CI to be green. The current failure in global CI is due to Todd's changes and I have a fix for them here konveyor/ci#182. Here is the pass rate right now: |
3 participants
API Tests PR: 375
Global CI PR:
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.