Change Tomcat selection to be driven by manifest pin.

[mshlyukarski]

Related to issue: #1155.

[ramonskie]

Code Review

Thanks for this PR — the root cause is correctly identified and the fix is well-scoped. We also confirm we already had JDK-based Tomcat selection in place (the >= 11 → 10.x / < 11 → 9.x logic), but the user's explicit JBP_CONFIG_TOMCAT version pin was never consulted. Below are our findings.

---

🔴 Critical: DetermineTomcatVersion extracts the wrong digit in many real-world configs

The regex (\d+) runs on the entire JBP_CONFIG_TOMCAT string and returns the first digit it finds — not necessarily the Tomcat version. This silently selects the wrong Tomcat version for common configs:

Example 1 — access logging only (no version pin):

JBP_CONFIG_TOMCAT="{access_logging_support: {access_logging: enabled}}"

→ regex matches "1" from "enabled" → returns "1.x" → selection overrides JDK-based logic with a nonsense version

Example 2 — external configuration enabled:

JBP_CONFIG_TOMCAT="{tomcat: {external_configuration_enabled: true}, ...}"

→ regex matches "1" from "true" → returns "1.x" → same problem

In both cases the function should return "" (no pin), but instead returns a garbage result that silently takes over the JDK-based selection path.

Suggested fix — parse specifically from the tomcat.version key, or better yet use YAML parsing (consistent with how the Ruby buildpack reads JBP_CONFIG_TOMCAT):

// Example: targeted regex instead of greedy first-digit match
re := regexp.MustCompile(`tomcat\s*:\s*\{[^}]*version\s*:\s*["']?(\d+)`)
match := re.FindStringSubmatch(configEnv)

---

🔴 Critical: The error return value of DetermineTomcatVersion is never checked

tomcatVersion, err := common.DetermineTomcatVersion(os.Getenv("JBP_CONFIG_TOMCAT"))
// err is never inspected before using tomcatVersion
if tomcatVersion == "" { ...

The function signals failure via err, but the call site only checks tomcatVersion == "". The error return is misleading — callers will assume a non-empty tomcatVersion means a valid result was parsed, which is not true (see Critical #1 above). Either check the error and log a warning, or change the signature to return only a string (empty = no pin found).

---

🟡 Warning: DetermineTomcatVersion belongs in tomcat.go, not common/context.go

The function is Tomcat-specific parsing of JBP_CONFIG_TOMCAT. The common package is for shared buildpack infrastructure (Context, Stager, Manifest interfaces, VCAP helpers). Placing a component-specific config parser there pollutes the shared package.

All the other JBP_CONFIG_TOMCAT parsers (isAccessLoggingEnabled, extractRepositoryRoot, extractVersion) already live in tomcat.go — this function should too.

---

🟡 Warning: No compatibility guard when the user pins an incompatible combination

The Tomcat compatibility matrix states Tomcat 10 requires Java 11+. If a user explicitly pins version: "10.+" with Java 8, the new code silently allows it — Tomcat will fail to start at runtime, not at staging. A warning during staging would help:

if tomcatMajor == 10 && javaMajorVersion < 11 {
    t.context.Log.Warning("Tomcat 10.x requires Java 11+, but Java %d detected. Tomcat may fail to start.", javaMajorVersion)
}

---

🔵 Suggestion: No unit tests for DetermineTomcatVersion

Given the function has non-trivial parsing behaviour, unit tests are essential. There is currently no context_test.go. Key cases to cover:

Input	Expected output
""	""
"{tomcat: { version: \"9.+\" }}"	"9.x"
"{access_logging_support: {access_logging: enabled}}"	"" ← currently broken
"{tomcat: {external_configuration_enabled: true}}"	"" ← currently broken
"{tomcat: { version: \"10.+\" }}"	"10.x"

---

✅ What the PR does well

• Correctly identifies and targets the root cause
• Minimal change — doesn't refactor unrelated code
• The fallback chain is preserved: explicit pin → JDK-based selection → DefaultVersion
• Integration tests are meaningful and test the right scenario
• Log message now consistently shows both Tomcat version and Java version

---

The fix is on the right track, but the DetermineTomcatVersion regex needs to be made precise before merge to avoid silently breaking users who set other JBP_CONFIG_TOMCAT options without a version pin.