Skip to content

Conversation

@mgoworko
Copy link
Collaborator

@mgoworko mgoworko commented Nov 15, 2025

Fix for WIndows-based CI jobs. Added retrying, when the downloaded ES zip is somehow corrupted or for any other reason cannot be unzipped.

Summary by CodeRabbit

  • Bug Fixes

    • Fixed JAR copying to preserve entry timestamps and ensure entries are properly closed.
  • New Features

    • Added a method to clean download artifacts.
  • Chores

    • Added retry with optional per-attempt cleanup and backoff to download/unzip and startup flows for improved reliability.
    • Added runtime logging around assembly for better visibility.
    • Reduced exposed test-utility surface by removing an unnecessary public path.
    • Bumped plugin version.

@mgoworko mgoworko marked this pull request as ready for review November 15, 2025 14:57
@coderabbitai

This comment was marked as resolved.

coderabbitai[bot]

This comment was marked as resolved.

@mgoworko mgoworko requested a review from coutoPL November 17, 2025 20:51
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/misc/ScalaUtils.scala (1)

78-100: Consider renaming the times parameter for clarity.

The parameter name times could be ambiguous. The implementation treats it as the total number of attempts (not retries), so calling retry(3, ...) makes 3 attempts total. Consider renaming to maxAttempts to clarify this behavior.

Example:

-def retry(times: Int, cleanBeforeRetrying: => Unit = ())(action: => Unit): Unit = {
+def retry(maxAttempts: Int, cleanBeforeRetrying: => Unit = ())(action: => Unit): Unit = {
📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

Disabled knowledge base sources:

  • Jira integration is disabled by default for public repositories

You can enable these sources in your CodeRabbit configuration.

📥 Commits

Reviewing files that changed from the base of the PR and between 6fd5c24 and 5bc378e.

📒 Files selected for processing (4)
  • tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsDirectoryManager.scala (1 hunks)
  • tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsSetup.scala (2 hunks)
  • tests-utils/src/main/scala/tech/beshu/ror/utils/misc/EsStartupChecker.scala (2 hunks)
  • tests-utils/src/main/scala/tech/beshu/ror/utils/misc/ScalaUtils.scala (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsDirectoryManager.scala
  • tests-utils/src/main/scala/tech/beshu/ror/utils/containers/windows/WindowsEsSetup.scala
🧰 Additional context used
🧬 Code graph analysis (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/misc/EsStartupChecker.scala (1)
tests-utils/src/main/scala/tech/beshu/ror/utils/misc/ScalaUtils.scala (2)
  • ScalaUtils (31-132)
  • retryBackoff (102-114)
🔇 Additional comments (4)
tests-utils/src/main/scala/tech/beshu/ror/utils/misc/EsStartupChecker.scala (2)

28-28: LGTM!

Clean import enabling use of the centralized retry utility.


39-39: LGTM! Linear retry strategy confirmed.

The backOffScaler = 1 parameter means the delay stays constant at 2 seconds (no exponential increase). This appears intentional to maintain the original behavior. Total wait time aligns with the 5-minute timeout on line 42.

tests-utils/src/main/scala/tech/beshu/ror/utils/misc/ScalaUtils.scala (2)

87-94: Logging verbosity is high but acceptable for test utilities.

Each retry logs the full exception stack trace (line 90), which can be quite verbose. However, given this is in test utilities and the PR aims to debug CI issues, this level of detail is likely intentional and helpful for troubleshooting intermittent failures.


22-31: The Try import is actively used—no change needed.

Line 45 uses Try(format.parse(value)).toOption.isDefined, confirming the import on line 29 is necessary.

Likely an incorrect or invalid review comment.

Copy link
Collaborator

@coutoPL coutoPL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mgoworko mgoworko merged commit e7cc39c into sscarduzio:develop Nov 18, 2025
2 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants