update Ktor EAP trigger settings: consolidate sample settings creation and restoration logic, refine handling of plugin and non-plugin samples #186
Conversation
…n and restoration logic, refine handling of plugin and non-plugin samples
WalkthroughIntroduces EapRepositoryConfig and replaces inline EAP repository/settings generation with generated content; refactors sample settings helpers to createEAPSampleSettings/restoreEAPSampleSettings, updates buildEAPGradleSample/buildEAPGradlePluginSample to use new flows, and adds env-driven Ktor Gradle plugin handling in the init script. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (7)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.teamcity/src/subprojects/train/TriggerProjectSamplesOnEAP.kt (1)
146-206: Consider refactoring duplicated pluginManagement blocks.The pluginManagement repository configuration is nearly identical between plugin samples (lines 146-170) and regular samples (lines 193-206), with the only difference being the resolutionStrategy block for plugin samples. This duplication increases maintenance burden.
Consider extracting the common repository configuration into a variable or function to reduce duplication:
+ # Common plugin management repos + PLUGIN_REPOS=' + repositories { + maven { + name = "KtorEAP" + url = uri("https://maven.pkg.jetbrains.space/public/p/ktor/eap") + content { + includeGroup("io.ktor") + } + } + gradlePluginPortal() + mavenCentral() + maven("https://maven.pkg.jetbrains.space/public/p/compose/dev") + }' + # Create settings based on sample type if [ "$isPluginSample" = "true" ]; then # Plugin samples need build-logic and plugin includes cat > "${'$'}{SETTINGS_FILE}" << 'EOF' ... pluginManagement { - repositories { - maven { - name = "KtorEAP" - url = uri("https://maven.pkg.jetbrains.space/public/p/ktor/eap") - content { - includeGroup("io.ktor") - } - } - gradlePluginPortal() - mavenCentral() - maven("https://maven.pkg.jetbrains.space/public/p/compose/dev") - } +EOF + echo "$PLUGIN_REPOS" >> "${'$'}{SETTINGS_FILE}" + cat >> "${'$'}{SETTINGS_FILE}" << 'EOF' resolutionStrategy {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.teamcity/src/subprojects/train/TriggerProjectSamplesOnEAP.kt(5 hunks)
🔇 Additional comments (5)
.teamcity/src/subprojects/train/TriggerProjectSamplesOnEAP.kt (5)
24-103: LGTM! Robust version fetching with proper fallbacks.The init script creation handles plugin version fetching with appropriate error handling, timeouts, and a sed fallback when jq is unavailable. The Gradle init script properly enforces EAP versions and provides clear logging.
215-236: LGTM! Clean restoration logic.The backup restoration logic is simple and correct, properly handling both the backup-exists and no-backup cases.
264-288: LGTM! Correct path handling for plugin samples.The function correctly adjusts paths for the plugin sample directory structure (adding "samples/" prefix and using "../.." for wrapper path). The same empty string concern from
buildEAPGradleSampleapplies here for standalone mode.
105-288: Well-structured refactoring improves clarity.The consolidation from separate plugin-specific functions to parameterized EAP functions with the
isPluginSampleflag is a clean design that reduces code duplication while maintaining clear separation between plugin and regular sample configurations. The removal of thestandaloneparameter fromrestoreEAPSampleSettingsis appropriate since the path information is sufficient for restoration.
241-253: No issue found — code is correct.TeamCity's Kotlin DSL accepts empty strings for
workingDirandgradleWrapperPath—an emptyworkingDiruses the default/checkout directory, and an emptygradleWrapperPathuses standard wrapper behavior. The code in lines 241-253 correctly leverages this behavior whenstandalone=true, so no changes are needed.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.teamcity/src/subprojects/train/TriggerProjectSamplesOnEAP.kt (1)
105-204: Consider consolidating the duplicated settings generation logic.The plugin and regular sample branches generate nearly identical
settings.gradle.ktscontent. The only difference is the plugin resolution strategy block (lines 154-163). This duplication means repository configurations must be maintained in two places, increasing the risk of inconsistency.Consider refactoring to generate the common parts once, then conditionally append the plugin-specific resolution strategy:
- # Create settings based on sample type - if [ "$isPluginSample" = "true" ]; then - # Plugin samples need build-logic and plugin includes - cat > "${'$'}{SETTINGS_FILE}" << 'EOF' + # Create base settings content + cat > "${'$'}{SETTINGS_FILE}" << 'EOF' dependencyResolutionManagement { repositories { maven { name = "KtorEAP" url = uri("https://maven.pkg.jetbrains.space/public/p/ktor/eap") content { includeGroup("io.ktor") } } google() mavenCentral() maven("https://maven.pkg.jetbrains.space/public/p/compose/dev") } } pluginManagement { repositories { maven { name = "KtorEAP" url = uri("https://maven.pkg.jetbrains.space/public/p/ktor/eap") content { includeGroup("io.ktor") } } gradlePluginPortal() mavenCentral() maven("https://maven.pkg.jetbrains.space/public/p/compose/dev") } - +EOF + + # Add plugin-specific resolution strategy if needed + if [ "$isPluginSample" = "true" ]; then + cat >> "${'$'}{SETTINGS_FILE}" << 'EOF' + resolutionStrategy { eachPlugin { if (requested.id.id == "io.ktor.plugin") { val pluginVersion = System.getenv("KTOR_GRADLE_PLUGIN_VERSION") if (pluginVersion != null && pluginVersion.isNotEmpty()) { useVersion(pluginVersion) } } } } -} EOF - else - # Regular samples - simpler setup - cat > "${'$'}{SETTINGS_FILE}" << 'EOF' -dependencyResolutionManagement { - repositories { - maven { - name = "KtorEAP" - url = uri("https://maven.pkg.jetbrains.space/public/p/ktor/eap") - content { - includeGroup("io.ktor") - } - } - google() - mavenCentral() - maven("https://maven.pkg.jetbrains.space/public/p/compose/dev") - } -} - -pluginManagement { - repositories { - maven { - name = "KtorEAP" - url = uri("https://maven.pkg.jetbrains.space/public/p/ktor/eap") - content { - includeGroup("io.ktor") - } - } - gradlePluginPortal() - mavenCentral() - maven("https://maven.pkg.jetbrains.space/public/p/compose/dev") - } + fi + + # Close pluginManagement block + cat >> "${'$'}{SETTINGS_FILE}" << 'EOF' } EOF - fi
🧹 Nitpick comments (2)
.teamcity/src/subprojects/train/TriggerProjectSamplesOnEAP.kt (2)
229-253: Consider clarifying the empty string assignments for standalone builds.When
standaloneistrue, bothsamplePathandwrapperPathare set to empty strings. While this likely relies on TeamCity's default behavior (emptyworkingDiruses checkout root, emptygradleWrapperPathuses default wrapper), the intent could be made more explicit with a brief comment.Consider adding a comment to document the intended behavior:
val samplePath = if (!standalone) relativeDir else "" val wrapperPath = if (!standalone) ".." else "" + // For standalone builds, empty strings tell TeamCity to use checkout root and default wrapper location
255-279: Consider clarifying the empty string assignments for standalone builds.Similar to
buildEAPGradleSample, whenstandaloneistrue, bothsamplePathandwrapperPathare set to empty strings. A clarifying comment would make the intent explicit.Consider adding the same documentation as suggested for
buildEAPGradleSample:val samplePath = if (!standalone) "samples/$relativeDir" else "" val wrapperPath = if (!standalone) "../.." else "" + // For standalone builds, empty strings tell TeamCity to use checkout root and default wrapper location
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.teamcity/src/subprojects/train/TriggerProjectSamplesOnEAP.kt(4 hunks)
🔇 Additional comments (2)
.teamcity/src/subprojects/train/TriggerProjectSamplesOnEAP.kt (2)
24-103: LGTM! Well-structured init script with proper error handling.The function correctly handles fetching the latest Ktor Gradle Plugin version with appropriate timeouts and retries, and the init script properly forces EAP versions with clear error messages when KTOR_VERSION is missing.
206-227: LGTM! Clean restoration logic.The function properly handles both scenarios (restoring from backup or removing temporary file) with clear logging.
…on logic and streamline sample settings creation
No description provided.