In progress - [RORDEV-1453] Ubuntu apt-get based installation of ES in tests #1127
Conversation
📝 WalkthroughWalkthroughThis set of changes introduces support for two Elasticsearch installation types—official Docker image and Ubuntu-based apt installation—within the Docker image creation utilities. The Sequence Diagram(s)Elasticsearch Docker Image Creation FlowsequenceDiagram
participant User
participant Elasticsearch
participant DockerImageCreator
participant Plugin
User->>Elasticsearch: new Elasticsearch(config)
Elasticsearch->>Elasticsearch: toDockerImageDescription()
alt esInstallationType == EsDockerImage
Elasticsearch->>DockerImageCreator: create(from official ES image)
else esInstallationType == UbuntuDockerImageWithEsFromApt
Elasticsearch->>DockerImageCreator: create(from ubuntu:24.04, install ES via apt)
end
Elasticsearch->>Plugin: updateEsImage(image, config)
Plugin->>DockerImageCreator: modify image (copy files, set user, etc.)
DockerImageCreator->>DockerImageCreator: setCommandFrom(command)
DockerImageCreator-->>Elasticsearch: DockerImageDescription
Elasticsearch-->>User: DockerImageDescription
Plugin Update FlowsequenceDiagram
participant Elasticsearch
participant Plugin
participant DockerImageCreator
Elasticsearch->>Plugin: updateEsImage(image, config)
Plugin->>DockerImageCreator: modify image (copy config, set user, update keystore, etc.)
DockerImageCreator-->>Plugin: DockerImageDescription
Plugin-->>Elasticsearch: DockerImageDescription
Keystore Configuration LogicsequenceDiagram
participant Plugin
participant DockerImageCreator
Plugin->>DockerImageCreator: configureKeystore(image, config)
alt esInstallationType == EsDockerImage
DockerImageCreator->>DockerImageCreator: run keystore commands
else esInstallationType == UbuntuDockerImageWithEsFromApt
DockerImageCreator->>DockerImageCreator: remove keystore as root
DockerImageCreator->>DockerImageCreator: run keystore commands
DockerImageCreator->>DockerImageCreator: chown /etc/elasticsearch
end
DockerImageCreator-->>Plugin: DockerImageDescription
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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 (3)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/XpackSecurityPlugin.scala (1)
94-103: Consider safer keystore removal.The
rmcommand could fail if the keystore doesn't exist. Consider usingrm -ffor idempotent behavior.- image.user("root").run("rm /etc/elasticsearch/elasticsearch.keystore") + image.user("root").run("rm -f /etc/elasticsearch/elasticsearch.keystore")tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala (2)
104-104: Consider parameterizing Ubuntu version.Hardcoding Ubuntu 24.04 may require future updates. Consider making it configurable.
133-133: Remove commented code.Dead code should be removed rather than commented out.
- //.run("chown -R elasticsearch:elasticsearch /etc/elasticsearch && chmod -R go-rwx /etc/elasticsearch")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/DockerImageCreator.scala(2 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/DockerImageDescription.scala(3 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala(5 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestPlugin.scala(2 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestWithEnabledXpackSecurityPlugin.scala(1 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/XpackSecurityPlugin.scala(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/DockerImageCreator.scala (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/DockerImageDescription.scala (1)
user(49-51)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/XpackSecurityPlugin.scala (4)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala (7)
images(60-64)Elasticsearch(27-69)Elasticsearch(71-237)EsInstallationType(38-42)configDir(46-52)EsDockerImage(39-39)UbuntuDockerImageWithEsFromApt(41-41)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestWithEnabledXpackSecurityPlugin.scala (1)
updateEsImage(68-73)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestPlugin.scala (1)
updateEsImage(66-80)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/DockerImageDescription.scala (3)
copyFile(53-55)user(49-51)run(30-32)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es818x)
- GitHub Check: ror (Run all tests LICENSE)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (CVE check Job)
🔇 Additional comments (17)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/DockerImageCreator.scala (2)
36-37: Good integration of command support with user context.The hardcoded elasticsearch user and command handling are properly positioned in the Docker build flow.
92-96: Clean implementation of command handling.The extension method correctly folds over the command field and applies CMD directives using the DockerfileBuilder API.
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/DockerImageDescription.scala (2)
28-28: Appropriate addition of optional command field.The optional command field enables Docker CMD directive support while maintaining backward compatibility.
69-71: Consistent API pattern for command configuration.The setCommand method follows the established immutable update pattern used throughout the class.
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestWithEnabledXpackSecurityPlugin.scala (1)
68-72: Correct propagation of esConfig through plugin composition.The esConfig parameter is properly threaded through both composed plugins, maintaining consistency with the new installation type support.
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestPlugin.scala (4)
66-66: Proper signature update for installation type support.Adding esConfig parameter enables dynamic configuration directory handling based on installation type.
69-74: Correct parameterization of configuration directories.Using
configDir(esConfig)instead of hardcoded paths properly supports both Docker image and Ubuntu apt installation types.
75-77: Thoughtful user context management for file ownership.The user switching pattern (elasticsearch → copy config → root) ensures proper file ownership while maintaining container security boundaries.
128-136: Consistent esConfig usage in FIPS dependency handling.The updateFipsDependencies method correctly accepts and uses esConfig for configuration paths, maintaining consistency with the main update flow.
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/XpackSecurityPlugin.scala (4)
19-19: LGTM!Import changes properly support the new installation type functionality.
42-48: Proper adaptation to support multiple installation types.The method correctly uses the dynamic
configDirbased on the installation type and properly propagates the configuration.
105-128: Clean extraction of keystore configuration logic.Good refactoring to avoid code duplication between installation types.
133-133: Good addition of --force flag.Ensures idempotent keystore configuration by overwriting existing keys.
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala (4)
29-42: Well-structured installation type support.Good use of sealed trait for exhaustive pattern matching. The default value ensures backward compatibility.
46-50: Correct dynamic path resolution.Properly returns configuration directory based on installation type.
53-53: Consistent API update.Method signature properly updated to support configuration propagation.
148-148: Correct config propagation.Plugin installation properly passes the configuration parameter.
…ation-tests-for-apt-based-es
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithNoSecurity.scala (1)
42-47: Consider givingadditionalLogConsumera default.Most callers pass
None; adding a default would avoid ripple changes when the extra consumer isn’t needed.-def create(..., - additionalLogConsumer: Option[Consumer[OutputFrame]]): EsContainer = { +def create(..., + additionalLogConsumer: Option[Consumer[OutputFrame]] = None): EsContainer = {tests-utils/src/main/scala/tech/beshu/ror/utils/containers/logs/DockerLogsToStringConsumer.scala (1)
24-40: Memory-growth guard recommended.
StringBuildergrows unbounded; long-running tests could exhaust memory.
Consider trimming or rotating after N KB, or allow callers to set a size cap.- private val buffer = new StringBuilder() + private val buffer = new StringBuilder() + private val maxSizeInChars: Int = 1_000_000 // 1 MB cap – tune as needed ... - if (frame != null && frame.getUtf8String != null) { - buffer.append(frame.getUtf8String) + if (frame != null && frame.getUtf8String != null) { + if (buffer.length > maxSizeInChars) buffer.delete(0, buffer.length / 2) + buffer.append(frame.getUtf8String)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala (1)
79-80: Minor optimisation.
setLogConsumers((logConsumer :: Nil).asJava)allocates an extra list.
A tiny micro-optimisation:-esContainer.container.setLogConsumers((logConsumer :: Nil).asJava) +esContainer.container.setLogConsumers(java.util.Collections.singletonList(logConsumer))tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithXpackSecurity.scala (1)
48-50: MakeadditionalLogConsumeroptional with a default valueEvery caller must now explicitly pass
Noneif they do not care about extra log consumers, which is easy to forget and breaks binary-compatibility. Providing a default keeps the old call-sites intact and improves ergonomics.- additionalLogConsumer: Option[Consumer[OutputFrame]]): EsContainer = { + additionalLogConsumer: Option[Consumer[OutputFrame]] = None): EsContainer = {tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithRorSecurity.scala (1)
51-53: Same default argument concern as in the X-Pack variantFor parity and to avoid needless
Nonenoise, add a default:- additionalLogConsumer: Option[Consumer[OutputFrame]]): EsContainer = { + additionalLogConsumer: Option[Consumer[OutputFrame]] = None): EsContainer = {tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithRorAndXpackSecurity.scala (1)
95-99: Default foradditionalLogConsumerThe private
createhelper forces every call-site to provide this arg. Adding a default (as proposed in sibling objects) keeps the public API neat.- additionalLogConsumer: Option[Consumer[OutputFrame]]): EsContainer = { + additionalLogConsumer: Option[Consumer[OutputFrame]] = None): EsContainer = {ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala (2)
131-135:tailRecMbusy-loops – add a small back-off
tailRecMretries as fast as the CPU allows, hammering both JVM and Docker until ES is up.
Insert a shortTask.sleepto yield and avoid a hot loop:- Task.delay(createAdminClient) + Task.sleep(250.millis) *> Task.delay(createAdminClient)
85-87: Timeout thestop()call to avoid hanging test shutdownIf Docker gets wedged the suite will never finish. Mirror the 5-minute upper bound you use for startup:
- esContainer.stop().runSyncUnsafe() + esContainer.stop().runSyncUnsafe(5.minutes)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala(1 hunks)ror-tools/src/test/scala/tech/beshu/ror/tools/utils/ExampleEsWithRorContainer.scala(3 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsClusterProvider.scala(1 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala(2 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerCreator.scala(8 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithNoSecurity.scala(2 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithRorAndXpackSecurity.scala(7 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithRorSecurity.scala(3 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithXpackSecurity.scala(3 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala(5 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestPlugin.scala(2 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestWithEnabledXpackSecurityPlugin.scala(1 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/logs/CompositeLogConsumer.scala(1 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/logs/DockerLogsToStringConsumer.scala(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/logs/CompositeLogConsumer.scala
🚧 Files skipped from review as they are similar to previous changes (3)
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestWithEnabledXpackSecurityPlugin.scala
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/ReadonlyRestPlugin.scala
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala
🧰 Additional context used
🧬 Code Graph Analysis (4)
ror-tools/src/test/scala/tech/beshu/ror/tools/utils/ExampleEsWithRorContainer.scala (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala (5)
images(60-64)Elasticsearch(27-69)Elasticsearch(71-236)EsInstallationType(38-42)EsDockerImage(39-39)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/logs/DockerLogsToStringConsumer.scala (2)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/logs/CompositeLogConsumer.scala (1)
accept(25-27)ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala (1)
getLogs(129-129)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/logs/CompositeLogConsumer.scala (1)
CompositeLogConsumer(24-28)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithRorAndXpackSecurity.scala (2)
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/ElasticsearchNodeWaitingStrategy.scala (4)
ElasticsearchNodeWaitingStrategy(29-60)ElasticsearchNodeWaitingStrategy(62-69)AwaitingReadyStrategy(65-68)WaitForEsReadiness(66-66)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala (3)
EsContainer(40-64)EsContainer(66-99)init(68-90)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (CVE check Job)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests IT_es818x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests LICENSE)
🔇 Additional comments (9)
ror-tools/src/test/scala/tech/beshu/ror/tools/utils/ExampleEsWithRorContainer.scala (3)
29-29: Import addition is spot-on.Required to reference the new ADT – no issues spotted.
112-113: Parameter now explicit – OK.Explicitly passing
Nonekeeps the call sites future-proof and documents intent.
95-103:❓ Verification inconclusive
Mind the trailing comma – Scala 2.12 compatibility risk.
Trailing commas after the last argument became legal only with the
-Ytrailing-commasflag on ≤2.12.
If this test code must compile on 2.12 (e.g. for older plugin versions or sbt cross-build), drop the final comma to stay portable.- esInstallationType = EsInstallationType.EsDockerImage, + esInstallationType = EsInstallationType.EsDockerImage
🏁 Script executed:
#!/bin/bash set -e echo "== crossScalaVersions ==" rg -n "crossScalaVersions" . echo echo "== scalaVersion ==" rg -n "scalaVersion" . echo echo "== Occurrences of Scala 2.12 ==" rg -n "2\\.12" .Length of output: 96
Mind the trailing comma – Scala 2.12 compatibility risk
Trailing commas on the last argument became valid only with the
-Ytrailing-commasflag in Scala 2.12 and below. If your build ever targets Scala 2.12 (e.g. in a cross-build or older plugin), drop the comma on line 103:- esInstallationType = EsInstallationType.EsDockerImage, + esInstallationType = EsInstallationType.EsDockerImagePlease verify your
scalaVersion/crossScalaVersionssettings to ensure you’re not compiling this test against Scala 2.12.tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsClusterProvider.scala (1)
66-75:✅ Verification successful
Same trailing-comma caveat applies here.
For Scala 2.13+ you’re fine; for 2.12 you’d need the compiler flag. If cross-compiled, remove the comma after
None.- additionalLogConsumer = None, + additionalLogConsumer = None
Let’s verify the project’s Scala versions to know if we need to drop the trailing comma:
🏁 Script executed:
#!/usr/bin/env bash set -eo pipefail # Look for cross-compilation settings rg -n "crossScalaVersions" . # Look for the main Scala version rg -n "scalaVersion" .Length of output: 47
Let’s search specifically in SBT files and ignore “no match” exits so we can see both settings:
🏁 Script executed:
#!/usr/bin/env bash # Don’t stop on no-match; show both settings if present rg -n "crossScalaVersions" --glob "*.sbt" || true rg -n "scalaVersion" --glob "*.sbt" || trueLength of output: 171
No change needed: project uses Scala 2.13.3
The onlyscalaVersionwe found is 2.13.3 (custom-audit-examples/ror-custom-scala-serializer/build.sbt:3), and Scala 2.13+ supports trailing commas in argument lists. You can leave the comma afterNoneas is.tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithNoSecurity.scala (2)
20-26: Imports are accurate.New parameter types are correctly imported.
53-54: Good delegation toEsContainer.init.Passing the new argument keeps logging plumbing centralized.
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala (1)
71-77: Composite-consumer logic looks solid.Neatly avoids duplicate registration and keeps default logging intact.
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerWithRorAndXpackSecurity.scala (1)
62-63: Trailing commas may not compile on 2.12 without the SIP-27 flagIf the build still targets Scala 2.12 and doesn’t enable
-Xsource:2.13, these trailing commas trigger a syntax error.
Double-check the Scala version/flags or simply drop the commas:- additionalLogConsumer = additionalLogConsumer, + additionalLogConsumer = additionalLogConsumerAlso applies to: 83-84
tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainerCreator.scala (1)
48-49: Nice! Parameter is optional with sane defaultAdding
esInstallationTypeandadditionalLogConsumeras defaulted params keeps the top-level API stable. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala (2)
24-26: Prefer consistent matcher importsMixing
org.scalatest.matchers.must.Matchers.includewithshould-style assertions is surprising and risks pulling in the wrong overloads. Import the matcher helpers from the same should package you’re already using forshould / shouldNot.-import org.scalatest.matchers.must.Matchers.include -import org.scalatest.matchers.should.Matchers.{should, shouldNot} +import org.scalatest.matchers.should.Matchers.{include, should, shouldNot}
141-144:Option.getcan explode during cleanup
esContainer.imageFromDockerfile.get()will throw if the image build failed, masking the original test error and preventing proper cleanup. Preferforeachorfoldto protect the shutdown path.- _ <- Task.delay(dockerClient.removeImageCmd(esContainer.imageFromDockerfile.get()).withForce(true).exec()) + _ <- Task.delay( + esContainer.imageFromDockerfile.foreach { id => + dockerClient.removeImageCmd(id).withForce(true).exec() + } + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala(1 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala(3 hunks)tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/EsContainer.scala
- tests-utils/src/main/scala/tech/beshu/ror/utils/containers/images/Elasticsearch.scala
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es818x)
- GitHub Check: ror (Run all tests LICENSE)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (CVE check Job)
ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala
Show resolved
Hide resolved
ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala (2)
88-100: Logs lost on failure – pull them infinallyThe earlier review comment still applies: if
start()or the test body blows up,getLogsis skipped. Capture logs in thefinallyblock instead.- try { - ... - esContainer.getLogs - } finally { - esContainer.stop().runSyncUnsafe() - } + var logs = "" + try { + ... + } finally { + logs = esContainer.getLogs + esContainer.stop().runSyncUnsafe() + } + logs
148-152: Busy-wait spin – add a small back-off
Task.tailRecMretries instantly, pegging a CPU if ES is slow to boot. Re-use the earlier suggestion:- Task.tailRecM(()) { _ => - Task.delay(createAdminClient) + Task.tailRecM(()) { _ => + Task + .delay(createAdminClient) + .delayExecution(250.millis) }
🧹 Nitpick comments (1)
integration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/support.scala (1)
41-45: Eliminate duplicatedafterAll()blocks via a single mix-inThe four traits repeat the same boiler-plate (
super.afterAll(); pruneDockerImages()). Consider extracting this into a dedicated trait, e.g.DockerPruningAfterAll, mixed in once per suite. Less repetition → fewer chances to forget the call and easier future maintenance.Also applies to: 59-63, 80-84, 96-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
integration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/support.scala(2 hunks)ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ror (Run all tests IT_es67x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es80x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es818x)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests LICENSE)
- GitHub Check: ror (CVE check Job)
| private def pruneDockerImages(): Unit = { | ||
| val dockerClient: DockerClient = DockerClientFactory.instance().client() | ||
| dockerClient.pruneCmd(PruneType.IMAGES).withDangling(false).exec() | ||
| } |
There was a problem hiding this comment.
pruneCmd(...).withDangling(false) nukes all local images – risky in shared CI runners
Forcing removal of every image (even non-dangling) can break other jobs running on the same daemon and wastes cache layers. Usually only dangling test images need pruning:
- dockerClient.pruneCmd(PruneType.IMAGES).withDangling(false).exec()
+ // Remove only unused layers; keep cached images for speed & safety
+ dockerClient.pruneCmd(PruneType.IMAGES).withDangling(true).exec()If full pruning is really required, gate it behind an env-flag (e.g. ROR_FORCE_PRUNE_ALL=1).
📝 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.
| private def pruneDockerImages(): Unit = { | |
| val dockerClient: DockerClient = DockerClientFactory.instance().client() | |
| dockerClient.pruneCmd(PruneType.IMAGES).withDangling(false).exec() | |
| } | |
| private def pruneDockerImages(): Unit = { | |
| val dockerClient: DockerClient = DockerClientFactory.instance().client() | |
| // Remove only unused layers; keep cached images for speed & safety | |
| dockerClient.pruneCmd(PruneType.IMAGES).withDangling(true).exec() | |
| } |
🤖 Prompt for AI Agents
In
integration-tests/src/test/scala/tech/beshu/ror/integration/suites/base/support.scala
around lines 109 to 112, the pruneDockerImages method currently removes all
local Docker images by using withDangling(false), which is risky on shared CI
runners. Modify the method to prune only dangling images by setting
withDangling(true). If full pruning is necessary, add a check for an environment
variable like ROR_FORCE_PRUNE_ALL=1 to conditionally enable pruning all images;
otherwise, default to pruning only dangling images.
ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala
Show resolved
Hide resolved
ror-tools/src/test/scala/tech/beshu/ror/tools/PatchingOfAptBasedEsInstallationSuite.scala
Show resolved
Hide resolved
| this: Suite with EsClusterProvider with ESVersionSupport => | ||
|
|
||
| override lazy val container: EsClusterContainer = clusterContainer | ||
|
|
||
| override protected def afterAll(): Unit = { | ||
| super.afterAll() | ||
| pruneDockerImages() |
There was a problem hiding this comment.
Adding the new test with apt-based ES caused the jobs to fail - not enough space on runner. Probably caused by downloading ~700 MB of ES package. In the first step I added cleaning of the resulting Docker images in newly added tests, but it was not enough. So I added pruning unused Docker images after each test suite.
|
little warning that I'm not sure if still valid: deb images were not available as soon as the archives in the past. It was a delayed thing. #TODO check if it's still the case |
Summary by CodeRabbit