[RORDEV-1435] Improved patching algorithm #1095
Conversation
mgoworko
force-pushed
the
feature/rordev-1435-improved-patching-algorithm
branch
from
52965d0 to
67157a1
Compare
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala
Show resolved
Hide resolved
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala
Show resolved
Hide resolved
| @@ -81,7 +84,29 @@ class RorToolsAppSuite extends AnyWordSpec with BeforeAndAfterAll with BeforeAnd | |||
| |""".stripMargin | |||
| ) | |||
| } | |||
| "Patching does not start when user declines to accept implications of patching (in arg)" in { | |||
| "Patching successful first time, on second try not started because already patched" in { | |||
There was a problem hiding this comment.
I think that the tests now cover all cases from diagrams https://github.com/sscarduzio/readonlyrest_kbn/pull/581
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Yes, it seems that all relevant cases are covered. There are some kbn-specific cases, because the patching looks different there from what I see. It operates on some kind of metadata/checksums, and those mechanisms are not used in ES.
There was a problem hiding this comment.
It seems I haven't stated it clearly. The new ROR KBN algorithm introduced the metadata file (we already have this in ES ROR as a "patched by" file). But we don't have checksums. Sometimes our users broke the installation manually, and we don't know if our patcher works well or not. That's why we introduced the checksums (please take a look at the comments in the ROR KBN PR). Let's try to add it here too.
📝 WalkthroughWalkthroughThe pull request updates various configurations and source files across multiple components. In the build pipeline ( Sequence Diagram(s)sequenceDiagram
participant User
participant RorToolsApp
participant EsPatchExecutor
participant BackupService
User->>RorToolsApp: Initiate patch operation
RorToolsApp->>EsPatchExecutor: create(esDirectory)
EsPatchExecutor->>EsPatchExecutor: checkWithPatchedByFileAndEsPatch()
alt Patch not applied
EsPatchExecutor->>BackupService: performBackup()
BackupService-->>EsPatchExecutor: Backup completed
EsPatchExecutor->>EsPatchExecutor: performPatching()
EsPatchExecutor-->>RorToolsApp: Return success or error
else Already patched or error
EsPatchExecutor-->>RorToolsApp: Return error
end
RorToolsApp->>RorToolsApp: handleResult(result)
sequenceDiagram
participant User
participant DockerEntrypoint
participant System
User->>DockerEntrypoint: Run container with env variable set
DockerEntrypoint->>DockerEntrypoint: Check for I_UNDERSTAND_AND_ACCEPT_ES_PATCHING
alt Variable exists
DockerEntrypoint->>DockerEntrypoint: Set CONFIRMATION from I_UNDERSTAND_AND_ACCEPT_ES_PATCHING
else Variable missing
DockerEntrypoint->>DockerEntrypoint: Check I_UNDERSTAND_IMPLICATION_OF_ES_PATCHING
DockerEntrypoint->>DockerEntrypoint: Set CONFIRMATION accordingly
end
DockerEntrypoint->>DockerEntrypoint: Lowercase CONFIRMATION and search for "yes"
alt "yes" found
DockerEntrypoint->>System: Check user ID (root access)
alt Not root
System-->>DockerEntrypoint: Request root access
else Root
DockerEntrypoint->>System: Execute patch command
end
else "yes" not found
DockerEntrypoint->>User: Output error message about patching consent
DockerEntrypoint-->>System: Exit with status code 2
end
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 3
🧹 Nitpick comments (10)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (2)
19-19: Consider adding doc comment for the base error classAdding a brief ScalaDoc comment for the
RorToolsErrorclass would improve code documentation and help developers understand its purpose and expected usage.+/** + * Base class for all ReadonlyREST tool errors. + * Used for standard error conditions that should be reported with a message but without stack traces. + */ sealed class RorToolsError(val message: String)
28-29: Consider using case object instead of objectSince
EsNotPatchedErrorextends a class and doesn't have any parameters, usingcase objectinstead of justobjectwould be more consistent with the pattern used for the other error types. This also provides better serialization support.-object EsNotPatchedError +case object EsNotPatchedError extends RorToolsError(s"Elasticsearch is NOT patched. ReadonlyREST cannot be used yet. For patching instructions see our docs: $patchingDocumentationUrl")docker-image/ror-entrypoint.sh (2)
7-13: Maintain backward compatibility while clarifying usage.The fallback to
I_UNDERSTAND_IMPLICATION_OF_ES_PATCHINGsupports older environment setups. Ensure documentation clearly states which variable is preferred (I_UNDERSTAND_AND_ACCEPT_ES_PATCHING) and whether the older variable remains fully supported or is deprecated.
15-15: Consider using an exact match for “yes”.Using
*yes*as a partial match can lead to unexpected acceptance if other text includes “yes.” An exact match check, such as=~ ^[yY][eE][sS]$, might reduce false positives.-if [[ "${CONFIRMATION,,}" == *"yes"* ]]; then +if [[ "${CONFIRMATION,,}" =~ ^[yY][eE][sS]$ ]]; thenror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala (2)
43-44: Surface stack traces or deeper context in errors if needed.While wrapping in
Try(...).toEitherhelps, you could log more details or preserve the exception cause inCannotVerifyIfPatchedfor easier debugging. Consider including stack traces when diagnosing patch issues.
52-55: Enhancement to capture verification failures.Defining
CannotVerifyIfPatchedextends error granularity, which is beneficial. For thorough debugging, consider referencing or logging the root cause or stack trace in addition to the message string.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (2)
41-43: Consider clarifying version naming.
This sealed hierarchy is clear, but it might be worth verifying thatexpectedRorVersionvs.patchedByRorVersionnaming aligns with real-world expectations (local vs. actual patch versions). A short doc comment or javadoc might help future maintainers.
46-60: Evaluate maintainability of large version-based matches.
Although this is effective, the chain of version checks can grow long over time. Consider a lookup table or a more modular approach if new versions are added frequently.ror-tools/src/test/scala/tech/beshu/ror/tools/RorToolsAppSuite.scala (2)
59-59: Improved test description clarity.The rephrasing from "Patching is successful for..." to "Patching successful for..." makes the test description more concise without changing meaning.
72-72: Improved test description clarity.Similar to the previous change, this rephrasing improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
azure-pipelines.yml(1 hunks)docker-image/ror-entrypoint.sh(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/actions.scala(0 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsPatchLoggingDecorator.scala(0 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala(2 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsException.scala(1 hunks)ror-tools/src/main/scala/tech/beshu/ror/tools/RorToolsApp.scala(3 hunks)ror-tools/src/test/scala/tech/beshu/ror/tools/RorToolsAppSuite.scala(11 hunks)ror-tools/src/test/scala/tech/beshu/ror/tools/utils/CapturingOutputAndMockingInput.scala(1 hunks)
💤 Files with no reviewable changes (2)
- ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsPatchLoggingDecorator.scala
- ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/actions.scala
🧰 Additional context used
🧬 Code Definitions (6)
ror-tools/src/test/scala/tech/beshu/ror/tools/utils/CapturingOutputAndMockingInput.scala (1)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (1)
readLine(43-43)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (4)
EsPatch(37-62)IsPatched(40-44)No(43-43)create(46-61)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (5)
EsPatchExecutor(26-112)EsPatchExecutor(114-120)verify(66-75)isPatched(77-77)create(115-119)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (4)
RorToolsError(19-19)RorToolsError(21-34)EsNotPatchedError(28-29)EsPatchedWithDifferentVersionError(25-26)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (1)
ror-tools/src/test/scala/tech/beshu/ror/tools/utils/CapturingOutputAndMockingInput.scala (1)
readLine(34-34)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala (1)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala (4)
performPatching(65-73)performBackup(53-55)performRestore(57-63)patchIsApplied(35-51)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala (5)
patches(26-80)patchIsApplied(35-51)performPatching(65-73)performBackup(53-55)performRestore(57-63)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala (5)
patches(24-35)patchIsApplied(33-33)performPatching(27-27)performBackup(29-29)performRestore(31-31)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala (5)
patches(26-60)patchIsApplied(35-41)performPatching(52-54)performBackup(43-45)performRestore(47-50)
ror-tools/src/main/scala/tech/beshu/ror/tools/RorToolsApp.scala (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (5)
EsPatchExecutor(26-112)EsPatchExecutor(114-120)patch(30-47)restore(49-64)verify(66-75)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (5)
InOut(33-45)ConsoleInOut(34-44)readLine(43-43)println(37-37)printlnErr(41-41)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (2)
RorToolsError(19-19)RorToolsError(21-34)
⏰ 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_es810x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (CVE check Job)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests LICENSE)
🔇 Additional comments (49)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsException.scala (1)
25-37: Improved exception handling structureThe refactored exception hierarchy is more informative and flexible than the previous implementation. By replacing the static objects with parameterized classes, you've improved error reporting capabilities:
EsPatchingNotRequirednow includes the specific ES version in the error messageEsPathExceptionprovides helpful guidance with documentation referenceThis approach is more maintainable and follows better Scala practices for error representation.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala (4)
27-27: Method signature correctly matches other patch implementationsThe
performPatchingmethod properly throwsEsPatchingNotRequiredwith the ES version, consistent with the other patch implementations likeSimpleEsPatchandTransportNetty4AwareEsPatch.
29-29: Backup method implementation is consistentThe
performBackupimplementation matches the pattern used in other patch classes, throwing the appropriate exception when patching is not required.
31-31: Restore method implementation is appropriateThe
performRestoremethod correctly throwsEsPatchingNotRequiredwhen ES doesn't need patching, maintaining consistency with the class's purpose.
33-33: New patch status check method added correctlyThe
patchIsAppliedmethod signature matches other implementations, taking the requiredcurrentRorVersionparameter and returningIsPatched. This aligns with the refactoring across the patching system.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala (6)
35-51: Method signature and implementation improvementsThe refactoring from
isPatchedtopatchIsAppliedwith the addition of thecurrentRorVersionparameter creates a more explicit contract. The implementation has been simplified by directly returning the appropriateIsPatchedvariant rather than using helper methods. This makes the code more straightforward and easier to understand.
41-42: Simplified error state representationGood simplification by using
IsPatched.Nodirectly rather than a more complex nested expression. This makes the code more readable while maintaining the same logical outcome.
46-49: Improved error message formattingThe reformatted error message maintains the same essential information but presents it in a clearer, more structured way. This will help users better understand the recovery process when corruption is detected.
53-55: Method renaming and implementation simplificationThe renaming to
performBackupbetter describes the action being taken. The implementation has been simplified, removing what appears to have been error recovery logic. Ensure that error handling is properly addressed elsewhere in the codebase if needed.
57-63: Clarified method name for restoration processThe rename from
restoretoperformRestoremaintains consistency with other method names and better describes its action-oriented nature.
65-73: Method renaming improves semantic clarityChanging
executetoperformPatchingmakes the method's purpose immediately clear without having to read its implementation. This is a good practice for maintaining readable and self-documenting code.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala (5)
35-41: Method renamed and signature updated for clarityThe renaming from
isPatchedtopatchIsAppliedimproves readability and aligns with verb-first naming conventions. The addition of thecurrentRorVersionparameter provides valuable context, and the simplified return type hierarchy makes the code more maintainable by removing unnecessary complexity.
43-45: Method renamed with simplified implementationThe rename to
performBackupclearly indicates the action being performed. However, I notice that error handling has been removed. This is likely acceptable if error handling has been moved to the callingEsPatchExecutorclass (mentioned in the summary), but worth confirming that errors are still appropriately caught and handled somewhere in the workflow.
47-50: Method renamed for consistencyThe rename from
restoretoperformRestoreprovides better naming consistency across the patching-related methods, making the API more intuitive.
52-54: Method renamed for clarityChanging from
executetoperformPatchingmakes the purpose of the method more explicit and follows the consistent naming pattern established across the class.
56-59: Return type explicitly specifiedExplicitly specifying the
Unitreturn type improves code clarity. This is a good practice in Scala to make the side-effecting nature of the method clear.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (2)
1-34: Well-structured error hierarchy for ReadonlyREST toolsThe implementation provides a clean, structured approach to error handling with clear, actionable error messages. Using a sealed class hierarchy is preferable to exceptions for these expected error conditions, which aligns with the comment that these are now "handled as errors - with error message and exit code 1, but without throwing exception."
The error messages are informative and include a documentation link, which is excellent for user experience.
23-23: Good practice: URL constant extractionExtracting the documentation URL as a constant is a good practice to avoid duplication and make future updates easier.
ror-tools/src/test/scala/tech/beshu/ror/tools/utils/CapturingOutputAndMockingInput.scala (1)
34-34:✅ Verification successful
Ensure all call-sites handle
Nonegracefully.Switching the return type from
StringtoOption[String]prevents exceptions whenmockedInputis empty. Verify that all test code now expectsOption[String]and properly handles theNonecase to avoid unintended behavior.Please run the following script to identify and review all usage of
readLine()in your tests:
🏁 Script executed:
#!/bin/bash # Search for references to readLine in the test suite and show surrounding lines for context fd -e scala --exec rg -A 5 "readLine" {}Length of output: 920
Action: Confirmed proper usage of Option-based
readLine()The search output shows that test call-sites—such as the
inOut.readLine() match { ... }block that handles bothSome("")andNone—appropriately address the change to anOption[String]return type. This pattern confirms that the tests expect and correctly manage theNonecase.
- Verified usage in test code handles the
Nonescenario via pattern matching.- Ensured that the transition from returning a
Stringto anOption[String]is accommodated in at least one major call-site.Please take a final manual look to ensure any other uses of
readLine()in tests follow a similar pattern.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala (2)
20-24: Imports align well with new patch architecture.The addition of
IsPatched,EsPatchExecutor, and error classes is consistent with the updated patching workflow. This improves code clarity by explicitly referencing the relevant data structures and error types.
32-39: Clear separation of patch states.Matching on
IsPatchedto distinguish between current version, different version, and no patch yields better error messages. The approach is straightforward and aligns with the newEsPatchExecutorlogic.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (2)
21-22: Imports look appropriate.
No issues identified with these new imports.
27-33: Consistent method naming.
Introducing explicit methodspatchIsApplied,performPatching,performBackup, andperformRestoreimproves clarity.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (5)
26-28: Using a final class is a solid choice.
Marking this class asfinalis good for composition and prevents unintended subclassing.
66-75: Straightforward verification.
The logging of patch status is clear. No concerns here.
77-78: Exposing patch status is useful.
Allowing other components to check the patch state is valuable. No issues found.
90-111: Method merges two checks elegantly.
Combining the local version check withesPatch.patchIsAppliedis reasonable. Keep an eye on complexity if additional conditions arise.
114-120: Factory method looks good.
Simple creation ofEsPatchExecutorusingEsPatch.create; no concerns identified.azure-pipelines.yml (1)
35-35:❓ Verification inconclusive
Upgrade to Ubuntu 22.04.
Please verify compatibility of tools and dependencies in this newer environment to avoid unexpected regressions.
🏁 Script executed:
#!/bin/bash # Check for potential missing OS dependencies or version issues echo "Listing apt packages that could differ between Ubuntu 20.04 and 22.04..." sudo apt-get update -qq sudo apt-get install -yqq apt-show-versions apt-show-versions | grep -E 'libc|java|docker'Length of output: 573
Manual Dependency Verification for Ubuntu 22.04 Upgrade
Upgrading the CI image to Ubuntu 22.04 is on track. However, note that our automated dependency-check script couldn’t execute as expected (it encountered missing “sudo” and related commands). Please perform a manual verification in an appropriate environment to confirm that key dependencies—such as libc, Java, and Docker—are compatible and that the system remains stable post-upgrade.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (2)
30-30: Improved error handling with Option[String] return type.The change from returning
StringtoOption[String]is a more idiomatic Scala approach that properly handles null or empty inputs. This improvement makes the code more robust against potential NPEs and provides a clearer API contract.
43-43: Good implementation of the Option wrapper.Correctly uses
Option(StdIn.readLine())to handle potential null values returned byStdIn.readLine(). This implementation aligns well with the method signature change in the trait.ror-tools/src/test/scala/tech/beshu/ror/tools/RorToolsAppSuite.scala (11)
75-75: Updated response parameter to match the new Option[String] return type.This change is necessary to align with the
readLine()method signature change in theInOuttrait, ensuring the test can correctly mock user input.
87-107: Added comprehensive test case for repeated patching scenario.This new test case properly verifies that patching fails when attempted on an already patched ES installation. The test provides good coverage for a real-world use case.
123-123: Updated response parameter to Option[String].Consistent change to align with the updated method signature.
133-162: Added test case for handling empty user input.This test ensures the system handles cases where the user provides empty input or the console input is not possible, enhancing the robustness of the application.
190-204: Added test case for patching when a patched_by file already exists.This test verifies the system correctly handles pre-existing patch indicators, ensuring proper error messages are displayed to users attempting to patch an already patched installation with a different version.
205-232: Added test for unpatching when patched_by file is missing.This test is valuable as it verifies the system can still perform unpatching operations even when the marker file is missing, adding flexibility to the recovery process.
233-247: Added test for unpatching failure with different version.This test ensures the system correctly prevents unpatching when the installation was patched with a different version, protecting against potential inconsistencies.
248-258: Added verification test for unpatched installations.This test confirms the verification command correctly detects when an installation is not patched, providing a clear message to users.
260-284: Added verification test for patched installations.Complements the previous test by verifying the detection of patched installations, ensuring the verification command is comprehensive.
286-311: Added verification test for missing patched_by file.This test ensures that verification correctly identifies unpatched status when the marker file is missing, regardless of actual patch status.
368-368: Updated method signature to use Option[String].This change is necessary to support the modified
readLine()method in theInOuttrait, ensuring test utilities can properly mock user input scenarios.ror-tools/src/main/scala/tech/beshu/ror/tools/RorToolsApp.scala (7)
22-23: Refactored to use EsPatchExecutor.The import change reflects a shift to a more organized approach using the
EsPatchExecutorclass for patch operations, promoting better separation of concerns.
105-106: Explicit failure handling for AnswerNotGiven case.This change improves clarity by explicitly handling the case when no answer is given, rather than falling through to the rejection case.
113-113: Simplified patching with EsPatchExecutor.Replaced direct patching actions with a call to the new
EsPatchExecutor.create(esDirectory).patch()method, promoting cleaner code and better encapsulation of patching logic.
124-134: Enhanced user input handling.The improved implementation properly handles various input scenarios:
- Explicitly checks for empty input (
Some("")) and no input (None)- Provides a clear error message to guide users when console input is unavailable
- Recommends using the silent mode with appropriate parameters
This makes the application more robust in different execution environments.
142-142: Simplified unpatching with EsPatchExecutor.Similar to the patching change, this uses the new
EsPatchExecutorfor unpatching operations, maintaining consistency in the codebase.
149-149: Simplified verification with EsPatchExecutor.Consistent with the other changes, this uses the new
EsPatchExecutorfor verification operations.
154-163: Added centralized error handling.This new method encapsulates error handling logic, improving code organization and ensuring consistent error presentation across different operations. The pattern matching on
Either[RorToolsError, Unit]aligns with functional programming principles and makes error flows more explicit.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala
Show resolved
Hide resolved
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala
Outdated
Show resolved
Hide resolved
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (1)
30-30: Improved API with Option type for better null handlingChanging the return type from
StringtoOption[String]is a good decision. This makes the possibility of no input explicit in the type system rather than relying on potentially null values, which aligns with Scala best practices. Note that this is a breaking change that will require updates to all calling code.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsException.scala (1)
25-37: Well-structured exception reorganizationThe refactoring of exception classes into the
RorToolsExceptionobject namespace improves code organization. The new classes provide more specific error handling with helpful context -EsPatchingNotRequiredindicates version-specific patching requirements andEsPathExceptionoffers guidance about the--es-pathoption with documentation links.One minor inconsistency:
EsPatchingNotRequiredis a regular class whileEsPathExceptionis a case class. Consider making them consistent, preferably both as case classes for equality semantics and pattern matching support.- final class EsPatchingNotRequired(esVersion: SemVer) + final case class EsPatchingNotRequired(esVersion: SemVer)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (1)
46-60: Simplified factory method with cleaner version matchingThe
createmethod has been simplified by removing an implicit parameter, making the API more straightforward. The pattern matching for Elasticsearch versions follows a logical progression from newer to older versions.You might consider extracting these version constants to a dedicated object for better maintainability and reuse across the codebase.
// Consider creating a dedicated object for version constants + object EsVersions { + val es8150: SemVer = ??? + val es8140: SemVer = ??? + // other version constants + }docker-image/ror-entrypoint.sh (1)
15-31: Clarify Consent Check and Error MessagingThe case-insensitive check using
${CONFIRMATION,,}is a robust way to handle user input. However, the error message still refers only toI_UNDERSTAND_IMPLICATION_OF_ES_PATCHING, which may confuse users now that the new variable is accepted. Consider updating the message to mention that setting eitherI_UNDERSTAND_AND_ACCEPT_ES_PATCHINGorI_UNDERSTAND_IMPLICATION_OF_ES_PATCHINGis acceptable.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (1)
79-88: Consider enhancing backup error handlingThe backup method also throws exceptions on failure rather than returning errors in a more Scala-idiomatic way.
While this is caught by the calling method, a more functional approach would be to return an Either type directly.
-private def backup(): Unit = { +private def backup(): Either[RorToolsError, Unit] = { inOut.println("Creating backup ...") Try(esPatch.performBackup()) match { case Success(_) => rorPluginDirectory.updatePatchedByRorVersion() + Right(()) case Failure(ex) => rorPluginDirectory.clearBackupFolder() - throw ex + Left(new RorToolsError(s"Failed to create backup: ${ex.getMessage}") {}) } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
azure-pipelines.yml(1 hunks)docker-image/ror-entrypoint.sh(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/actions.scala(0 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsPatchLoggingDecorator.scala(0 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala(2 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsException.scala(1 hunks)ror-tools/src/main/scala/tech/beshu/ror/tools/RorToolsApp.scala(3 hunks)ror-tools/src/test/scala/tech/beshu/ror/tools/RorToolsAppSuite.scala(11 hunks)ror-tools/src/test/scala/tech/beshu/ror/tools/utils/CapturingOutputAndMockingInput.scala(1 hunks)
💤 Files with no reviewable changes (2)
- ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/actions.scala
- ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsPatchLoggingDecorator.scala
🧰 Additional context used
🧬 Code Definitions (8)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala (2)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (4)
EsPatchExecutor(26-112)EsPatchExecutor(114-120)verify(66-75)isPatched(77-77)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (4)
RorToolsError(19-19)RorToolsError(21-34)EsNotPatchedError(28-29)EsPatchedWithDifferentVersionError(25-26)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (1)
ror-tools/src/test/scala/tech/beshu/ror/tools/utils/CapturingOutputAndMockingInput.scala (1)
readLine(34-34)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala (1)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala (4)
performPatching(65-73)performBackup(53-55)performRestore(57-63)patchIsApplied(35-51)
ror-tools/src/main/scala/tech/beshu/ror/tools/RorToolsApp.scala (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (5)
EsPatchExecutor(26-112)EsPatchExecutor(114-120)patch(30-47)restore(49-64)verify(66-75)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (5)
InOut(33-45)ConsoleInOut(34-44)readLine(43-43)println(37-37)printlnErr(41-41)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (2)
RorToolsError(19-19)RorToolsError(21-34)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/RorPluginDirectory.scala (4)
patches(23-88)doesBackupFolderExist(32-34)isTransportNetty4PresentInRorPluginPath(56-58)findTransportNetty4Jar(60-62)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/filePatchers/ElasticsearchJarPatchCreator.scala (3)
patches(23-29)patches(31-38)create(26-29)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (2)
create(46-61)No(43-43)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (4)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/RorPluginDirectory.scala (1)
patches(23-88)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala (5)
patches(24-35)patchIsApplied(33-33)performPatching(27-27)performBackup(29-29)performRestore(31-31)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala (5)
patches(26-60)patchIsApplied(35-41)performPatching(52-54)performBackup(43-45)performRestore(47-50)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/EsUtil.scala (2)
EsUtil(22-76)readEsVersion(43-57)
ror-tools/src/test/scala/tech/beshu/ror/tools/utils/CapturingOutputAndMockingInput.scala (1)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (1)
readLine(43-43)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/RorPluginDirectory.scala (4)
patches(23-88)updatePatchedByRorVersion(70-73)readCurrentRorVersion(75-85)readPatchedByRorVersion(64-68)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (6)
EsPatch(37-62)IsPatched(40-44)No(43-43)WithCurrentVersion(41-41)WithDifferentVersion(42-42)create(46-61)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (5)
RorToolsError(19-19)RorToolsError(21-34)EsAlreadyPatchedError(31-32)EsPatchedWithDifferentVersionError(25-26)EsNotPatchedError(28-29)
⏰ 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_es810x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (CVE check Job)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests LICENSE)
🔇 Additional comments (53)
ror-tools/src/test/scala/tech/beshu/ror/tools/utils/CapturingOutputAndMockingInput.scala (1)
34-34: Improved return type to better handle absent inputThe change from returning
StringtoOption[String]is a good functional approach that aligns with the similar change inInOuttrait. This eliminates exception throwing for absent input and makes the API more explicit about possible outcomes. Callers will now need to handle theNonecase, which leads to more robust error handling.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (1)
43-43: Properly implemented Option wrappingThe implementation correctly wraps
StdIn.readLine()withOption(), which handles the case wherereadLine()might return null. This approach is consistent with the test code inCapturingOutputAndMockingInput.scalaand ensures null safety throughout the codebase.azure-pipelines.yml (1)
35-35: Update VM Image to Ubuntu 22.04The change on line 35 updates the build pool's VM image from Ubuntu 20.04 to Ubuntu 22.04. This is a welcome upgrade, as it should provide improved security, better toolchain support, and overall performance gains. Please verify that all build scripts and dependencies continue to function correctly under the new OS version.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala (1)
27-33: LGTM - Improved method naming and consistent behaviorThe refactoring in this class maintains a consistent pattern of throwing
EsPatchingNotRequiredacross all methods, with renamed methods that clearly express their intent. The new methodpatchIsAppliedfollows the same pattern and completes the interface implementation.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala (5)
35-41: Method renaming and implementation simplification looks goodThe change from
isPatchedtopatchIsAppliedbrings clarity to the method's purpose, and the simplified logic using the newIsPatchedtype system is cleaner. The implementation now relies on the existence of a backup folder rather than checking a specific file, which seems more robust.
43-45: Verify error handling for backup operationsThe method has been renamed from
backuptoperformBackupwhich is consistent with the naming pattern in this PR. However, it appears that error handling was removed. This method no longer uses Try/Success/Failure to handle potential errors from the backup process.Please confirm that error handling has been moved to a higher level in the call stack, possibly in the new
EsPatchExecutormentioned in the PR summary. Without proper error handling, failed backups might go undetected.
47-50: Rename looks good for restore operationThe method renaming from
restoretoperformRestoremaintains consistency with other method names in this class.
52-54: Rename looks good for patch operationThe method renaming from
executetoperformPatchingimproves clarity about what the method actually does.
56-59: Return type change needs verificationThe private method
copyJarsToBackupFoldernow returnsUnitinstead of an implicitTry. This aligns with the simplified error handling approach, but it's important to ensure errors are properly propagated and handled elsewhere.Please confirm that errors during the backup folder creation or during the backup process are properly detected and handled at a higher level. This is especially important for operations that might fail due to file system permissions or disk space issues.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (3)
21-22: Import organization improvedThe imports have been properly reorganized to explicitly reference required classes from the internal package. This is preferable to wildcard imports as it makes dependencies clearer.
27-33: Method names are now more descriptiveThe renamed methods (
patchIsApplied,performPatching,performBackup,performRestore) provide better clarity about their purpose compared to the previous names. The addition of thecurrentRorVersionparameter topatchIsAppliedmakes it explicit that version checking is part of determining patch status.
41-43: Improved patch status representationThe redesigned
IsPatchedhierarchy now explicitly captures different patching scenarios through dedicated case classes rather than generic Yes/No states. This makes it clearer how to handle version mismatches versus unpatched states.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala (3)
20-24: Improved imports reflect new architectureThe new imports align with the refactored patching mechanism, using the centralized
EsPatchExecutorand the more expressiveIsPatchedenumeration. This change supports better error handling with specific error types.
32-39: Improved error handling with detailed patch statusThe code now uses the more granular
IsPatchedenum, providing clearer information about the patching state. The pattern matching handles all possible states:
- Successfully patched with current version
- Patched with a different version (requiring repatching)
- Not patched at all
This approach gives users more precise error messages, making troubleshooting easier.
43-44: Method renamed to match new architectureThe method rename from
createPatchertocreateEsPatchExecutorbetter reflects its purpose and aligns with the new centralized patching architecture. The implementation now properly delegates to theEsPatchExecutorfactory method.Note that this is a public API change as mentioned in the summary.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala (7)
26-28: Class signature looks goodThe constructor parameters are well-organized with appropriate type declarations.
35-39: Method renamed for better clarity, implementation simplifiedGood renaming from
isPatchedtopatchIsAppliedwhich makes the intent clearer. The implementation has been simplified to directly returnIsPatched.WithCurrentVersion(currentRorVersion)instead of an additional method call, which streamlines the code.
40-42: Simplified return typeThe return statement has been simplified from
No(Cause.NotPatchedAtAll)toIsPatched.No, which aligns with the new enumeration structure for representing patch state.
46-49: Improved error message clarityThe error message now provides clearer instructions for recovery from a corrupted state, including a link to download Elasticsearch binaries.
53-55: Method renamed for consistencyGood rename from
backuptoperformBackupfor better consistency with other method names in the codebase. This helps maintain a unified naming convention.
57-63: Method renamed for consistencyThe
restoremethod has been renamed toperformRestorewhile maintaining the same implementation logic. This improves naming consistency across the codebase.
65-73: Method renamed for clarity and consistencyRenaming from
executetoperformPatchingmakes the method's purpose more explicit and maintains consistent naming with other methods. The implementation remains unchanged.docker-image/ror-entrypoint.sh (1)
7-13: Environment Variable Precedence Updated CorrectlyThe updated block properly prioritizes
I_UNDERSTAND_AND_ACCEPT_ES_PATCHINGand falls back toI_UNDERSTAND_IMPLICATION_OF_ES_PATCHINGif the new variable isn’t set. This addresses the previously noted omission for the ES 6x entrypoint.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (1)
17-34: Well-structured error hierarchy for patching operationsThis new file introduces a clean, hierarchical structure for error handling in the ROR tools, centralizing error types and messages. The sealed class pattern with specific case classes is ideal for exhaustive pattern matching in the application code.
The error messages are informative, providing clear explanations and documentation links where appropriate. This approach is much better than throwing exceptions with generic messages.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (4)
17-25: Clean imports and good dependency organizationYour imports are well-organized and specific. The class dependencies are clearly identified.
66-77: Good implementation of verify methodThe
verify()method cleanly outputs the patching status without changing state. The pattern matching is clear and handles all possible states correctly.
90-111: Comprehensive patching verification logicThe
checkWithPatchedByFilemethod is thorough in checking various patching states. It correctly handles cases where the patched_by file is missing or contains a different version.I appreciate the clear progression through the checks:
- Check if file exists and its content
- Determine patching state based on version comparison
- Validate with the actual patch implementation
114-120: Good factory method implementationThe companion object's
createmethod provides a clean factory pattern for instantiating the executor, hiding implementation details from consumers.ror-tools/src/test/scala/tech/beshu/ror/tools/RorToolsAppSuite.scala (18)
21-21: Updated import for better test assertionsAdding the
bematcher enhances readability of assertions.
39-42: Improved test directory structure variablesRestructuring the path variables enhances clarity and makes the test setup more understandable.
59-59: Clearer test descriptionUpdated test description is more concise and precise about what's being tested.
72-72: Clearer test descriptionSimilar improvement to the test description here.
75-75: Updated to handle Option[String] for inputUpdated to match the new API that uses Option[String] for user input, aligning with the implementation changes.
87-108: Comprehensive test for patching idempotenceGood test case that verifies the behavior when trying to patch an already patched installation. This covers an important edge case.
109-119: Test for rejecting patching consentThis test ensures the application correctly handles when a user rejects the patching consent through arguments.
120-132: Test for rejecting patching consent in interactive modeSimilar to the previous test, but verifies the interactive mode behavior.
133-147: Test for handling missing inputGood test case for when console input is not available, ensuring the application provides clear guidance to the user.
148-162: Test for empty inputThorough test covering the case where the user provides an empty string as input.
190-204: Test for handling different patch versionsGood test that verifies the application correctly identifies and reports when Elasticsearch is patched with a different ROR version.
205-232: Test for unpatching without patched_by fileThis test ensures that unpatching works even when the patched_by file is missing, which is an important resilience feature.
233-247: Test for handling incompatible versions during unpatchingThis test verifies that the application correctly prevents unpatching when ES is patched with a different version.
248-259: Test for verification when not patchedGood test for verifying the correct reporting when ES is not patched.
260-285: Test for verification when patchedComprehensive test that verifies the patching status detection works correctly with the patched_by file.
286-312: Test for verification when patch file is missingThis test ensures that verification correctly identifies an unpatched state when the patched_by file is missing.
340-340: Added validation for patched_by file existenceGood additional check that verifies the patched_by file exists after patching.
368-369: Updated method signature for Option[String]The method signature now correctly uses Option[String] for the response parameter, matching the implementation changes in the InOut trait.
ror-tools/src/main/scala/tech/beshu/ror/tools/RorToolsApp.scala (7)
22-24: Updated imports for refactored patching implementationThe imports reflect the migration from direct EsPatch usage to the new EsPatchExecutor pattern, improving separation of concerns.
105-107: Explicit handling of AnswerNotGiven caseThe code now explicitly handles the case when no answer is given, making the control flow clearer.
113-113: Migration to EsPatchExecutorGood refactoring to use the new EsPatchExecutor, centralizing patching logic in a single component.
124-134: Improved handling of empty or missing user inputThe implementation now properly handles both empty strings and null input, providing clear guidance to the user when console input isn't available.
142-142: Migration to EsPatchExecutor for restore operationsConsistent with the patching changes, the restore operation now uses EsPatchExecutor.
149-149: Migration to EsPatchExecutor for verify operationsThe verify operation has also been updated to use EsPatchExecutor, maintaining consistency.
154-163: New handleResult method improves error handlingThe new method centralizes the logic for processing Either results from patching operations, making the code more consistent and maintainable.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala
Outdated
Show resolved
Hide resolved
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
docker-envs/base-ror-docker/es/Dockerfile-use-ror-binaries-from-file (1)
6-6: ARG declaration appears unused in this DockerfileThe
ROR_VERSIONargument is declared but not referenced in this Dockerfile. If it's needed for cross-file consistency or external build processes, this is fine, but consider adding a comment to clarify its purpose.docker-envs/base-ror-docker/es/Dockerfile-build-for-from-sources (1)
19-19: ARG declaration appears unused in this DockerfileThe
ROR_VERSIONargument is declared but not referenced in this Dockerfile. If it's needed for cross-file consistency or external build processes, this is fine, but consider adding a comment to clarify its purpose.docker-envs/base-ror-docker/run.sh (2)
26-26: Address shellcheck warning by separating declaration and assignmentThe current implementation masks any potential error from the grep command. Separate the declaration from the assignment to improve error handling.
- export ROR_ES_VERSION=$(grep -oP '^pluginVersion=\K.*' ../../../gradle.properties) + ROR_ES_VERSION=$(grep -oP '^pluginVersion=\K.*' ../../../gradle.properties) + export ROR_ES_VERSION🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 26-26: Declare and assign separately to avoid masking return values.
(SC2155)
60-60: Address shellcheck warning by separating declaration and assignmentSame issue as before - the current approach masks potential errors from the unzip or grep commands. Separate the declaration from the assignment.
- export ROR_ES_VERSION=$(unzip -p "$path" plugin-descriptor.properties | grep -oP '^version=\K.*') + ROR_ES_VERSION=$(unzip -p "$path" plugin-descriptor.properties | grep -oP '^version=\K.*') + export ROR_ES_VERSION🧰 Tools
🪛 Shellcheck (0.10.0)
[warning] 60-60: Declare and assign separately to avoid masking return values.
(SC2155)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docker-envs/base-ror-docker/es/Dockerfile-build-for-from-sources(1 hunks)docker-envs/base-ror-docker/es/Dockerfile-use-ror-binaries-from-file(1 hunks)docker-envs/base-ror-docker/run.sh(2 hunks)
🧰 Additional context used
🪛 Shellcheck (0.10.0)
docker-envs/base-ror-docker/run.sh
[warning] 26-26: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 60-60: Declare and assign separately to avoid masking return values.
(SC2155)
⏰ 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_es80x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests LICENSE)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (CVE check Job)
docker-envs/base-ror-docker/es/Dockerfile-build-for-from-sources
Outdated
Show resolved
Hide resolved
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala
Outdated
Show resolved
Hide resolved
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala
Outdated
Show resolved
Hide resolved
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala
Show resolved
Hide resolved
...core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala
Outdated
Show resolved
Hide resolved
...core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala
Show resolved
Hide resolved
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala
Show resolved
Hide resolved
| @@ -81,7 +84,29 @@ class RorToolsAppSuite extends AnyWordSpec with BeforeAndAfterAll with BeforeAnd | |||
| |""".stripMargin | |||
| ) | |||
| } | |||
| "Patching does not start when user declines to accept implications of patching (in arg)" in { | |||
| "Patching successful first time, on second try not started because already patched" in { | |||
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (1)
74-82: Same exception-throwing pattern as discussed previously.
This mirrors the established pattern of throwing exceptions for technical errors, which has been deemed acceptable in past discussions.
🧹 Nitpick comments (2)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (1)
54-54: Removed IsPatched trait and simplified status checkingBased on previous review comments, the
IsPatchedtrait with itsYesandNocase objects has been removed in favor of a boolean return type. This simplifies the API but loses the additional context about which ROR version applied the patch. If version information is still needed, consider adding a separate method likegetPatchedRorVersion(): Option[String]to retrieve this information when required.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (1)
30-44: Consider partial rollback on patch failure.
Currently, ifperformPatching()fails afterbackup()succeeds, the code does not automatically restore the previous state, possibly leaving Elasticsearch partially patched. Introducing an automatic rollback could enhance reliability.case EsPatchStatus.NotPatched => backup() inOut.println("Patching ...") try { esPatch.performPatching() inOut.println("Elasticsearch is patched! ReadonlyREST is ready to use") Right(()) } catch { + case exception: Throwable => + // Potential rollback + inOut.println("Patching failed. Attempting to restore from backup...") + doRestore() // or partial restore logic + Left(new RorToolsError(s"Patch exception: ${exception.getMessage}") {}) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala(1 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala
- ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala
🧰 Additional context used
🧬 Code Graph Analysis (4)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (6)
EsPatchExecutor(26-118)EsPatchExecutor(120-137)EsPatchStatus(124-130)verify(61-70)isPatched(72-72)NotPatched(129-129)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/InOut.scala (2)
InOut(33-45)ConsoleInOut(34-44)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (4)
RorToolsError(19-19)RorToolsError(21-34)EsNotPatchedError(28-29)EsPatchedWithDifferentVersionError(25-26)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (4)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala (5)
patches(23-53)isPatchApplied(32-34)performPatching(45-47)performBackup(36-38)performRestore(40-43)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/EsNotRequirePatch.scala (5)
patches(23-34)isPatchApplied(32-32)performPatching(26-26)performBackup(28-28)performRestore(30-30)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/RorPluginDirectory.scala (1)
patches(23-88)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/EsUtil.scala (2)
EsUtil(22-76)readEsVersion(43-57)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala (3)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala (5)
isPatchApplied(34-50)performBackup(52-54)copyJarsToBackupFolder(74-77)performRestore(56-62)performPatching(64-72)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/RorPluginDirectory.scala (3)
doesBackupFolderExist(32-34)restore(48-50)clearBackupFolder(40-42)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (2)
restore(46-59)patch(30-44)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (5)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala (5)
patches(23-53)performPatching(45-47)isPatchApplied(32-34)performRestore(40-43)performBackup(36-38)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/TransportNetty4AwareEsPatch.scala (5)
patches(25-79)performPatching(64-72)isPatchApplied(34-50)performRestore(56-62)performBackup(52-54)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/internal/RorPluginDirectory.scala (7)
patches(23-88)backup(44-46)restore(48-50)updatePatchedByRorVersion(70-73)clearBackupFolder(40-42)readCurrentRorVersion(75-85)readPatchedByRorVersion(64-68)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/utils/RorToolsError.scala (5)
RorToolsError(19-19)RorToolsError(21-34)EsAlreadyPatchedError(31-32)EsPatchedWithDifferentVersionError(25-26)EsNotPatchedError(28-29)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (2)
create(38-53)EsPatch(36-54)
⏰ 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_es80x)
- GitHub Check: ror (Run all tests IT_es70x)
- GitHub Check: ror (Run all tests IT_es810x)
- GitHub Check: ror (Run all tests IT_es710x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (Run all tests LICENSE)
- GitHub Check: ror (CVE check Job)
🔇 Additional comments (17)
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/PatchingVerifier.scala (3)
20-24: Updated imports for improved patching architectureThese import changes reflect the architectural shift from a simple binary patch check to a more granular patching status system. The inclusion of specific error types aligns with the more detailed error handling approach.
32-39: Improved pattern matching with more specific patching statesThe refactored code now handles three distinct states instead of a simple Yes/No, providing more precise error messaging. The pattern matching against the
EsPatchStatusenumeration is clean and exhaustive, making the code more robust and maintainable.
43-44: Method renamed for clarity and alignment with new architectureThe method name change from
createPatchertocreateEsPatchExecutorbetter reflects its purpose, and the implementation now correctly creates anEsPatchExecutorinstance using the ES directory. This aligns with the broader refactoring of the patching subsystem.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatch.scala (5)
19-22: Updated imports to match refactored architectureThe imports have been properly updated to include the necessary classes after the refactoring.
26-26: Method renamed for clarity and simplified return typeChanging from
isPatched: IsPatchedtoisPatchApplied: Booleansimplifies the API by using a boolean rather than a custom trait. This improves readability but removes the ability to determine which ROR version applied the patch. Consider whether this information is still needed elsewhere in the system.
28-32: Method names clarified to better represent their actionsThe method names have been updated to be more explicit about their actions, which improves readability and makes the API more intuitive:
execute()→performPatching()backup()→performBackup()restore()→performRestore()This aligns well with the verb-noun pattern for method naming best practices.
36-38: Parameter type improved for better type safetyChanging from
os.Pathto the domain-specificEsDirectorytype provides better type safety and encapsulation. This is a good design improvement that makes the API more self-documenting.
40-52: Pattern matching structure improved for ES version handlingThe pattern matching logic has been updated to handle different Elasticsearch versions more consistently. The code now follows a cleaner and more maintainable approach for selecting the appropriate patch implementation.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (8)
1-16: No issues in the license header block.
All looks good here.
26-28: Class structure is clear.
The constructor parameters and implicit inOut usage are consistent with common Scala patterns. No issues.
46-59: Restore logic looks consistent.
Domain-specific failures returnEitherwhile unexpected file system issues remain exceptions. This approach aligns with your design choice.
61-70: Simple and effective status reporting.
No issues detected. The printed messages are clear for the user.
72-73: Straightforward status accessor.
TheisPatchedmethod’s single-line logic is concise and easy to follow.
84-93: Consistent backup strategy with rollback on failure.
Clearing the backup folder on a backup failure and rethrowing the exception aligns with the approach used elsewhere.
95-117: Graceful handling of mismatched patch versions.
Logic is well-structured by returning domain errors for version mismatches and noting a correct patch status otherwise. No concerns here.
120-137: Factory object design is sound.
TheEsPatchExecutorcompanion object’s creation method neatly centralizes initialization, enhancing maintainability.ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/SimpleEsPatch.scala (1)
32-33: Confirm if checking only the backup folder is sufficient for patch detectionThis implementation of
isPatchAppliedrelies solely ondoesBackupFolderExist, which might cause false positives if the backup folder exists but the patch was never fully applied. InTransportNetty4AwareEsPatch.scala, we see additional checks for the transport netty4 jar. Consider mirroring that approach or adding further validation to avoid partial or corrupted states.
| override def performRestore(): Unit = { | ||
| filePatches.restore() | ||
| rorPluginDirectory.clearBackupFolder() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential restoration failures
filePatches.restore() or rorPluginDirectory.clearBackupFolder() might fail due to permission issues or other I/O errors, leaving a partially restored system. Consider capturing or propagating exceptions to ensure that failures are communicated and addressed rather than silently ignored.
| private def copyJarsToBackupFolder(): Unit = { | ||
| rorPluginDirectory.createBackupFolder() | ||
| filePatches.backup() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Prevent silent failures in jar backup
rorPluginDirectory.createBackupFolder() and filePatches.backup() can throw or fail. Without error handling or logging, backup failures would remain undetected. Consider reintroducing a return type like Try[Unit] or Either[RorToolsError, Unit] to handle errors consistently across all backup and restore methods.
| override def performBackup(): Unit = { | ||
| copyJarsToBackupFolder() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure error handling for backup operation
By removing a Try-based or similar error-handling approach, performBackup() now silently ignores potential I/O failures. This could result in incomplete backups without clear feedback. Consider returning an Either or throwing a dedicated exception to handle backup failures gracefully.
| override def performPatching(): Unit = { | ||
| filePatches.patch() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add failure handling for patching
If filePatches.patch() encounters an error, there is no retry or rollback strategy. Consider adopting an error model, such as returning an Either or throwing domain-specific exceptions, to prevent leaving the system in an inconsistent state.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
docker-envs/base-ror-docker/es/install-ror-es-using-api.sh (1)
14-17: 🛠️ Refactor suggestionInitial ROR_VERSION Validation
The check forROR_VERSIONat this stage is important; however, note that a very similar check appears later (lines 22-25). Consider removing one of the redundant validations to streamline the script.
🧹 Nitpick comments (3)
docker-envs/base-ror-docker/es/install-ror-es-using-file.sh (2)
16-16: Extraction of ROR_VERSION
The use ofunzip -pin combination withgrep -oPto extract the version is a neat solution. Consider handling potential extraction failures more explicitly or adjusting the error message below if the value ends up empty.
18-21: Clarify Error Message for Missing ROR_VERSION
The error message currently prints"No $ROR_VERSION variable is set", which resolves to an empty string whenROR_VERSIONis unset. A clearer message—for example,"ROR_VERSION is not set"—would improve usability.docker-envs/base-ror-docker/es/install-ror-es-using-api.sh (1)
22-25: Eliminate Duplicate ROR_VERSION Check
The block immediately after the plugin install duplicates the earlier validation forROR_VERSION. Removing this redundancy will simplify the script without sacrificing robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docker-envs/base-ror-docker/es/install-ror-es-using-api.sh(2 hunks)docker-envs/base-ror-docker/es/install-ror-es-using-file.sh(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
docker-envs/base-ror-docker/es/install-ror-es-using-api.sh (2)
docker-envs/base-ror-docker/es/install-ror-es-using-file.sh (1)
verlte(3-7)docker-envs/base-ror-docker/kbn/install-ror-kbn-using-api.sh (1)
verlte(3-5)
⏰ 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_es90x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests UNIT)
- GitHub Check: ror (CVE check Job)
- GitHub Check: ror (Run all tests LICENSE)
🔇 Additional comments (10)
docker-envs/base-ror-docker/es/install-ror-es-using-file.sh (4)
3-7: Version Comparison Function (verlte) is Clear and Consistent
The implementation using local variables and stripping pre-release suffixes is well done and consistent with similar scripts in the repository.
23-31: Java Binary Path Determination Based on ES_VERSION
The conditional logic correctly selects the appropriate Java binary path depending on the ES version. Verify that the version boundaries (7.0.0 and 6.7.0) consistently meet the deployment requirements.
33-38: Conditional Setting of Patch OPTIONS
Setting theOPTIONSvariable based on the ROR version is in line with the newer flag requirements. This clear-cut conditional ensures that the patch command receives the correct arguments.
40-41: Execution of the Patch Command
The final execution step correctly uses the dynamically determinedJAVA_BIN_PATHandOPTIONS. This straightforward approach is easy to follow and maintain.docker-envs/base-ror-docker/es/install-ror-es-using-api.sh (6)
3-7: Version Comparison Function (verlte) Review
This function implementation effectively strips pre-release parts and compares version strings, maintaining consistency with other scripts in the codebase.
9-12: ES_VERSION Validation Check
The script correctly verifies that theES_VERSIONvariable is set before proceeding, ensuring that an undefined variable doesn’t cause unexpected behavior.
19-20: Installation Command with Dynamic Parameters
Incorporating bothES_VERSIONandROR_VERSIONinto the download URL makes the installation dynamic and flexible. Ensure that these parameters are properly sanitized if there is any risk of malformed input.
27-35: Java Binary Path Determination
The conditional structure for setting theJAVA_BIN_PATHbased onES_VERSIONis consistently applied and mirrors the logic used in the file-based installation script.
37-42: Conditional Configuration of Patch OPTIONS
The selection of the--I_UNDERSTAND_AND_ACCEPT_ES_PATCHING=yesflag based on the ROR version is handled well, ensuring consistency with consent updates across the codebase.
44-46: Execution of the Patch Operation
The final command correctly invokes patching with the specified Java binary and conditionally set options, and the concluding message confirms successful execution.
| echo "Patching ES ROR $ROR_VERSION..." | ||
| /usr/share/elasticsearch/jdk/bin/java -jar /usr/share/elasticsearch/plugins/readonlyrest/ror-tools.jar patch --I_UNDERSTAND_AND_ACCEPT_ES_PATCHING yes | ||
|
|
||
| if [[ -z "$ROR_VERSION" ]]; then |
There was a problem hiding this comment.
shouldn't it be if [ ! -v ROR_VERSION ] || [ -z "$ROR_VERSION" ]; then?
`
There was a problem hiding this comment.
BTW, we have the same check above
|
|
||
| if [[ -z "$ROR_VERSION" ]]; then | ||
| echo "No $ROR_VERSION variable is set" | ||
| exit 2 |
| JAVA_BIN_PATH="$JAVA_HOME/bin/java" | ||
| else | ||
| echo "Unsupported ES version: $ES_VERSION" | ||
| exit 1 |
| JAVA_BIN_PATH="$JAVA_HOME/bin/java" | ||
| else | ||
| echo "Unsupported ES version: $ES_VERSION" | ||
| exit 1 |
|
|
||
| def patch(): Either[RorToolsError, Unit] = { | ||
| checkWithPatchedByFileAndEsPatch() match { | ||
| case EsPatchStatus.PatchedWithCurrentRorVersion(rorVersion) => |
There was a problem hiding this comment.
Let's do import EsPatchStatus.*.IMO The errors are clear without adding this context explicitly.
| case EsPatchStatus.PatchedWithOtherRorVersion(expectedRorVersion, patchedByRorVersion) => | ||
| Left(EsPatchedWithDifferentVersionError(expectedRorVersion, patchedByRorVersion)) | ||
| case EsPatchStatus.NotPatched => | ||
| backup() |
There was a problem hiding this comment.
Maybe we can introduce doPatch private method, as we did with doRestore? WDYT?
| doRestore() | ||
| case EsPatchStatus.PatchedWithOtherRorVersion(expectedRorVersion, patchedByRorVersion) => | ||
| Left(EsPatchedWithDifferentVersionError(expectedRorVersion, patchedByRorVersion)) | ||
| case EsPatchStatus.NotPatched if esPatch.isPatchApplied => |
There was a problem hiding this comment.
I think this smells. Does it mean we should have another status? Is it tested?
Maybe we have to describe this use case better.
| } | ||
| } | ||
|
|
||
| def verify(): Unit = { |
There was a problem hiding this comment.
AFAIR, in the KBN changes, I mentioned that we should return status code 0 when verify tells us the ES is patched. And otherwise return -1. It would be helpful when the patcher is used in scripts.
please taka a look at ROR KBN patcher tests.
| case Some(patchedByRorVersion) => | ||
| EsPatchStatus.PatchedWithOtherRorVersion(currentRorVersion, patchedByRorVersion) | ||
| } | ||
| resultOfCheckingPatchedByFile match { |
There was a problem hiding this comment.
Can we do it in one pattern match?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
ror-tools/src/main/scala/tech/beshu/ror/tools/RorToolsApp.scala (1)
121-134: Broaden accepted answers & trim I/O noise
- Only
"yes"(in full) is accepted. Users often typey,Y, or hit ENTER for “no”.- The additional
printlnbefore the silent mode hint inserts an empty line in the middle of stdout/err streams, which complicates CI log parsing.- inOut.readLine() match { - case Some("") | None => - inOut.println("\nIt seems that the answer was not given or the ror-tools are executed in the environment that does not support console input.") + inOut.readLine() match { + case Some("") | None => + inOut.println("It seems that the answer was not given or the ror-tools are executed in an environment that does not support console input.") … - case Some(line) => - line.toLowerCase match { - case "yes" => PatchingConsent.Accepted - case _ => PatchingConsent.Rejected + case Some(line) => + line.trim.toLowerCase match { + case "yes" | "y" => PatchingConsent.Accepted + case "no" | "n" => PatchingConsent.Rejected + case _ => PatchingConsent.Rejected }Tiny UX tweak, big happiness gain.
ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (1)
111-119:NotPatchedbranch hides partially‑patched installationsIf
patched_bymatches the current version butisPatchApplied == false, the executor concludesNotPatched.
That scenario is only reachable when the file was updated prematurely (see previous comment) or manually edited – both inconsistent states that deserve an explicit error (or at least a warning) instead of silently treating as unpatched.Consider introducing a dedicated status, e.g.
MetadataMismatch, and surface it to the caller.ror-tools/src/test/scala/tech/beshu/ror/tools/RorToolsAppSuite.scala (3)
124-131: Prefer a substring assertion to make the test less brittle
output should equal(...)will fail on any inconsequential whitespace mismatch or if additional log lines are ever added.
include(orstartWith/endWith) gives adequate safety while keeping the intent clear.-result should equal(Result.Failure) -output should equal( +result should equal(Result.Failure) +output should include((The same pattern recurs in subsequent blocks that assert on full‑string equality.)
146-160: Reduce flakiness in the “no console input” scenarioFor the block that builds the expected output with string concatenation (
stripMargin + " " + ...) the exact spacing/new‑line sequence is easy to mis‑count and will break if any prompt text changes.Consider switching to
include(or splitting the expectation into two separateincludechecks) instead of asserting full equality.
381-385: Unify the two helpers to cut duplication
captureResultAndOutputandcaptureResultAndOutputWithInteractiondiffer only by the optionalresponse.
A single method with a default argument keeps the suite shorter and easier to evolve:private def captureResultAndOutput( block: InOut => Result, response: Option[String] = None ): (Result, String) = { val inOut = new CapturingOutputAndMockingInput(response) val result = block(inOut) (result, inOut.getOutputBuffer) }Call sites that don’t need interaction stay unchanged.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docker-envs/base-ror-docker/es/install-ror-es-using-api.sh(1 hunks)docker-envs/base-ror-docker/es/install-ror-es-using-file.sh(2 hunks)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala(1 hunks)ror-tools/src/main/scala/tech/beshu/ror/tools/RorToolsApp.scala(3 hunks)ror-tools/src/test/scala/tech/beshu/ror/tools/RorToolsAppSuite.scala(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- docker-envs/base-ror-docker/es/install-ror-es-using-api.sh
🧰 Additional context used
🧬 Code Graph Analysis (1)
docker-envs/base-ror-docker/es/install-ror-es-using-file.sh (3)
tests-utils/src/main/scala/tech/beshu/ror/utils/elasticsearch/SearchManager.scala (1)
head(212-212)docker-envs/base-ror-docker/es/install-ror-es-using-api.sh (1)
verlte(3-7)ror-tools-core/src/main/scala/tech/beshu/ror/tools/core/patches/base/EsPatchExecutor.scala (1)
patch(31-41)
⏰ 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 LICENSE)
- GitHub Check: ror (Run all tests IT_es90x)
- GitHub Check: ror (Run all tests IT_es717x)
- GitHub Check: ror (Run all tests IT_es816x)
- GitHub Check: ror (CVE check Job)
| if verlte "7.0.0" "$ES_VERSION"; then | ||
| JAVA_BIN_PATH="/usr/share/elasticsearch/jdk/bin/java" | ||
| elif verlte "6.7.0" "$ES_VERSION"; then | ||
| JAVA_BIN_PATH="$JAVA_HOME/bin/java" | ||
| else | ||
| echo "Unsupported ES version: $ES_VERSION" | ||
| exit 4 |
There was a problem hiding this comment.
ES version check is always true – parameters are in the wrong order
verlte "7.0.0" "$ES_VERSION" returns true for all ES versions ≥ 7.0.0 (it checks whether 7.0.0 ≤ $ES_VERSION).
What we really need is “is the running ES ≤ 7.0.0?”.
Swap the arguments so the tested value comes first.
-if verlte "7.0.0" "$ES_VERSION"; then
+if verlte "$ES_VERSION" "7.0.0"; then
…
-elif verlte "6.7.0" "$ES_VERSION"; then
+elif verlte "$ES_VERSION" "6.7.0"; thenWithout this fix every ES 8.x container will wrongly pick the 7.x Java path.
📝 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 verlte "7.0.0" "$ES_VERSION"; then | |
| JAVA_BIN_PATH="/usr/share/elasticsearch/jdk/bin/java" | |
| elif verlte "6.7.0" "$ES_VERSION"; then | |
| JAVA_BIN_PATH="$JAVA_HOME/bin/java" | |
| else | |
| echo "Unsupported ES version: $ES_VERSION" | |
| exit 4 | |
| if verlte "$ES_VERSION" "7.0.0"; then | |
| JAVA_BIN_PATH="/usr/share/elasticsearch/jdk/bin/java" | |
| elif verlte "$ES_VERSION" "6.7.0"; then | |
| JAVA_BIN_PATH="$JAVA_HOME/bin/java" | |
| else | |
| echo "Unsupported ES version: $ES_VERSION" | |
| exit 4 |
| private def backup(): Unit = { | ||
| inOut.println("Creating backup ...") | ||
| Try(esPatch.performBackup()) match { | ||
| case Success(_) => | ||
| rorPluginDirectory.updatePatchedByRorVersion() | ||
| case Failure(ex) => | ||
| rorPluginDirectory.clearBackupFolder() | ||
| throw ex | ||
| } |
There was a problem hiding this comment.
Updating patched_by before patching creates false‑positive state after failed patch
backup() marks the installation as patched (updatePatchedByRorVersion) before performPatching() runs.
If the patch then crashes, the instance is not patched but patched_by says it is, causing:
• verify to report a success (DetectedPatchWithoutValidMetadata fallback)
• Subsequent patch runs to refuse with “already patched”.
Move the metadata update after a successful patch instead:
-private def backup(): Unit = {
- inOut.println("Creating backup ...")
- Try(esPatch.performBackup()) match {
- case Success(_) =>
- rorPluginDirectory.updatePatchedByRorVersion()
+private def backup(): Unit = {
+ inOut.println("Creating backup ...")
+ Try(esPatch.performBackup()) match {
+ case Success(_) =>
()and add the update at the end of doPatch():
esPatch.performPatching()
+rorPluginDirectory.updatePatchedByRorVersion()This keeps metadata and reality in sync.
Committable suggestion skipped: line range outside the PR's diff.
Summary by CodeRabbit
Chores
New Features
Bug Fixes
Refactor