MOEN-45889: Fix update-dependency workflow release type detection - #232
MOEN-45889: Fix update-dependency workflow release type detection#232kkgowtham-engg-sdk wants to merge 1 commit into
Conversation
|
🚪 Hodor is reviewing this PR... 👀 |
…e type from version diff update-bom.main.kts and update-ios-deps.main.kts always tagged changelog entries as [minor], even for patch or major bumps. Both now compare the old and new version numbers to pick the correct [major]/[minor]/[patch] tag consumed by the pre-release script.
fe75d88 to
716fd87
Compare
|
|
||
| // ── Semver Diff ────────────────────────────────────────────────────────────── | ||
|
|
||
| fun determineReleaseType(oldVersion: String, newVersion: String): String { |
There was a problem hiding this comment.
💡 Suggestion — Handle downgrade case: The current logic returns "patch" when neither major nor minor components change, which is correct for patch bumps (1.2.3 → 1.2.4) or no change (1.2.3 → 1.2.3). However, if a downgrade occurs (1.2.3 → 1.2.2), this would incorrectly return "patch".
While downgrades shouldn't happen in practice (Maven Central versions only go up), consider adding explicit validation:
fun determineReleaseType(oldVersion: String, newVersion: String): String {
val oldParts = oldVersion.split(".").map { it.toIntOrNull() ?: 0 }
val newParts = newVersion.split(".").map { it.toIntOrNull() ?: 0 }
val oldMajor = oldParts.getOrElse(0) { 0 }
val oldMinor = oldParts.getOrElse(1) { 0 }
val oldPatch = oldParts.getOrElse(2) { 0 }
val newMajor = newParts.getOrElse(0) { 0 }
val newMinor = newParts.getOrElse(1) { 0 }
val newPatch = newParts.getOrElse(2) { 0 }
return when {
newMajor != oldMajor -> if (newMajor > oldMajor) "major" else error("Version downgrade detected")
newMinor != oldMinor -> if (newMinor > oldMinor) "minor" else error("Version downgrade detected")
newPatch != oldPatch -> if (newPatch > oldPatch) "patch" else error("Version downgrade detected")
else -> error("No version change detected")
}
}This would catch configuration errors early. Not blocking since downgrades are unlikely, but improves robustness.
|
|
||
| // ── Semver Diff ────────────────────────────────────────────────────────────── | ||
|
|
||
| fun determineReleaseType(oldVersion: String, newVersion: String): String { |
There was a problem hiding this comment.
💡 Suggestion — Same downgrade handling as Android script: The iOS version has the same potential downgrade edge case as the Android script. Consider applying the same validation improvement suggested in update-bom.main.kts for consistency. See the inline comment on line 148 of update-bom.main.kts for the suggested approach.
There was a problem hiding this comment.
Review Summary
Branching & CFR: patch/MOEN-45889_native_sdk_release_type_fix — category: patch, impact: Neutral CFR ✅
This PR fixes a bug in the dependency update automation scripts where BOM/pod version bumps were always tagged as [minor] in changelog entries, regardless of the actual semver change. The fix adds a determineReleaseType(oldVersion, newVersion) helper that correctly classifies changes as major/minor/patch by comparing version components, and updates the regex patterns to detect all three release type tags.
Quality Gates
✅ PR Size: 70 lines across 2 files — good size
ℹ️ Tests: No test files modified (automation scripts — manual verification via workflow runs)
✅ Migrations: None
✅ Duplication: determineReleaseType is duplicated across both scripts (acceptable for small utility functions in separate workflow scripts)
All checks passed. ✅
Issues Found
🔴 Critical: 0
💡 Suggestion: 1
Key Findings
Logic Quality: The semver comparison logic is correct. The function properly handles:
- Major/minor/patch version bumps
- Malformed versions (missing components default to 0)
- Non-numeric suffixes like
-alpha(gracefully fall back to 0)
Regex Update: The regex patterns now correctly match all three release types [major], [minor], [patch] instead of only [minor], which was the root cause of re-run failures.
Integration: Verified that getReleaseTypeForModule in sdk-automation-scripts/scripts/common/utils.main.kts (lines 696-718) correctly parses these tags from CHANGELOG entries to determine release type.
What's Good
✅ Solves the stated bug — the release type will now correctly reflect the actual version change
✅ Logic is sound and handles edge cases well
✅ Backward compatible — existing [minor] entries still match the updated regex
✅ Consistent implementation across both Android (BOM) and iOS (pods) scripts
Verdict
The fix is solid. One minor suggestion about handling downgrade scenarios (which shouldn't occur in practice but would currently return "patch"). Otherwise, the logic correctly implements semver-based release type detection and integrates cleanly with the pre-release automation.
COMMENT — No blocking issues, ready to merge after optional improvement consideration.
Summary
update-bom.main.ktsandupdate-ios-deps.main.ktsalways tagged the changelog bom/pod-version bump as[minor], regardless of the actual semver change.determineReleaseType(oldVersion, newVersion)helper to both scripts that compares major/minor components of the old vs. new version and tags the entry[major],[minor], or[patch]accordingly.[minor].Test plan
[patch].[minor].[major].pre-release.main.ktspicks up the correct release type from the tagged entries.🤖 Generated with Claude Code