Add support for cnb (#1629) - LMCROSSITXSADEPLOY-2993 #1630

s-yonkov-yonkov · 2025-05-08T09:01:24Z

Add support for CNB

Original PR: #1560

* add CNB type * add validation for cnb * add validation and tests * update lifecycle to be nullable * refactoring * add tests * update validation

theghost5800 · 2025-06-04T14:54:55Z

...src/main/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParser.java

+        try {
+            return LifecycleType.valueOf(lifecycleValue.toUpperCase());
+        } catch (IllegalArgumentException e) {
+            throw new SLException("Unsupported lifecycle value: " + lifecycleValue);


Extract message into constant and avoid string concatenation, just pass lifecycleValue as second argument to SLException constructor. Actually this type of issue is related with user content, it's not error from our side, so this can be org.cloudfoundry.multiapps.common.ContentException instead SLException.

theghost5800 · 2025-06-04T14:57:19Z

...src/main/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParser.java

+    }
+
+    private void validateLifecycleType(LifecycleType lifecycleType, List<String> buildpacks, DockerInfo dockerInfo) {
+        if (lifecycleType == LifecycleType.CNB && (buildpacks == null || buildpacks.isEmpty())) {


Just use CollectionUtils to check for empty/null list.

theghost5800 · 2025-06-05T07:16:51Z

...src/main/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParser.java

+            throw new SLException("Buildpacks must be provided when lifecycle is set to 'cnb'.");
+        }
+        // Validate Docker-specific conditions
+        if (lifecycleType == LifecycleType.DOCKER) {
+            if (dockerInfo == null) {
+                throw new SLException("Docker information must be provided when lifecycle is set to 'docker'.");
+            }
+            if (buildpacks != null && !buildpacks.isEmpty()) {
+                throw new SLException("Buildpacks must not be provided when lifecycle is set to 'docker'.");
+            }
+        } else if (dockerInfo != null && lifecycleType != null) {
+            throw new SLException("Docker information must not be provided when lifecycle is set to " + lifecycleType + "'.");
+        }


Extract string messages into Messages class.

Replace throwing SLException with ContentException, this should avoid automatic retry from multiapps cli plugin when error is related with content of mta descriptor.

Try to avoid nested if blocks by extracting logic into new methods.

theghost5800 · 2025-06-05T07:19:01Z

...test/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParserTest.java

+    void testValidateLifecycleWithCnbAndValidBuildpacks() {
+        parametersList.add(Collections.singletonMap("lifecycle", "cnb"));
+        parametersList.add(Collections.singletonMap("buildpacks", List.of("custom-buildpack-url")));
+        assertDoesNotThrow(() -> parser.parse(parametersList));


Isn't it better to validate fields of parsed object that includes proper values?

theghost5800 · 2025-06-05T07:20:37Z

...test/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParserTest.java

+    @Test
+    void testValidateLifecycleWithDockerAndValidDockerInfo() {
+        parametersList.add(getDockerParams());
+        assertDoesNotThrow(() -> parser.parse(parametersList));


same comment

theghost5800 · 2025-06-05T07:21:07Z

...test/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParserTest.java

+    @Test
+    void testValidateLifecycleWithBuildpackAndNoBuildpacks() {
+        parametersList.add(Collections.singletonMap("lifecycle", "buildpack"));
+        assertDoesNotThrow(() -> parser.parse(parametersList));


same comment

stiv03 · 2025-06-05T07:21:41Z

...src/main/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParser.java

+        } catch (IllegalArgumentException e) {
+            throw new SLException("Unsupported lifecycle value: " + lifecycleValue);
+        }


Extract the error message as a constant and use MessageFormat.format instead of string concatenation

stiv03 · 2025-06-05T07:22:09Z

...src/main/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParser.java

+            if (dockerInfo == null) {
+                throw new SLException("Docker information must be provided when lifecycle is set to 'docker'.");
+            }
+            if (buildpacks != null && !buildpacks.isEmpty()) {
+                throw new SLException("Buildpacks must not be provided when lifecycle is set to 'docker'.");
+            }


Extract the error messages

stiv03 · 2025-06-05T07:22:59Z

...src/main/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParser.java

+        } else if (dockerInfo != null && lifecycleType != null) {
+            throw new SLException("Docker information must not be provided when lifecycle is set to " + lifecycleType + "'.");


Same here, extract message and use MessageFormat.format

stiv03 · 2025-06-05T07:25:50Z

...test/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParserTest.java

+        assertEquals("Buildpacks must be provided when lifecycle is set to 'cnb'.", exception.getMessage());
+    }


Extract the expected error message

stiv03 · 2025-06-05T07:28:13Z

...test/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParserTest.java

+        assertEquals("Buildpacks must not be provided when lifecycle is set to 'docker'.", exception.getMessage());
+    }


same here, extract the expected error message

stiv03 · 2025-06-05T07:33:32Z

...test/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParserTest.java

+    @Test
+    void testValidateLifecycleWithCnbAndValidBuildpacks() {
+        parametersList.add(Collections.singletonMap("lifecycle", "cnb"));
+        parametersList.add(Collections.singletonMap("buildpacks", List.of("custom-buildpack-url")));
+        assertDoesNotThrow(() -> parser.parse(parametersList));


extract strings like "buildpacks", "lifecycle" and "cnb" as constants to avoid duplication

stiv03 · 2025-06-05T07:38:11Z

...g/cloudfoundry/multiapps/controller/process/steps/CreateOrUpdateStepWithExistingAppTest.java

 				Arguments.of(ImmutableStaging.builder().addBuildpack("buildpack-1").command("command1").build(),
-						ImmutableStaging.builder().addBuildpack("buildpack-1").command("command2").build(), true),
+						ImmutableStaging.builder().addBuildpack("buildpack-1").command("command2").build(), true, LifecycleType.BUILDPACK),
 				Arguments.of(ImmutableStaging.builder().addBuildpack("buildpack-1").build(),
-						ImmutableStaging.builder().addBuildpack("buildpack-1").build(), false),
+						ImmutableStaging.builder().addBuildpack("buildpack-1").build(), false, LifecycleType.BUILDPACK),
 				Arguments.of(
 						ImmutableStaging.builder().addBuildpack("buildpack-1").command("command1").stackName("stack1")
 								.healthCheckTimeout(5).healthCheckType("process").isSshEnabled(false).build(),
 						ImmutableStaging.builder().addBuildpack("buildpack-2").command("command2").stackName("stack2")


extract repeated strings like "buildpack-1", "command1", "stack1" and others into constants

theghost5800

Before merge adopt released version of cf-java-client-sap and squash commits into one

IvanBorislavovDimitrov · 2025-06-09T06:12:32Z

...src/main/java/org/cloudfoundry/multiapps/controller/core/parser/StagingParametersParser.java

+            return null;
+        }
+        try {
+            return LifecycleType.valueOf(lifecycleValue.toUpperCase());


can you use the method LifecycleType::from ?

sonarqubecloud · 2025-06-09T12:47:41Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
98.1% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

s-yonkov-yonkov and others added 2 commits June 4, 2025 12:59

Add support for cnb (#1629)

624560f

* add CNB type * add validation for cnb * add validation and tests * update lifecycle to be nullable * refactoring * add tests * update validation

Apply formatting

629713f

s-yonkov-yonkov force-pushed the add-support-for-cnb branch from e86c145 to 629713f Compare June 4, 2025 09:59

Add lifecycle to supported parameters

f89fffb

theghost5800 reviewed Jun 5, 2025

View reviewed changes

stiv03 suggested changes Jun 5, 2025

View reviewed changes

Changes on comments

20d0dff

stiv03 approved these changes Jun 6, 2025

View reviewed changes

theghost5800 previously approved these changes Jun 6, 2025

View reviewed changes

IvanBorislavovDimitrov reviewed Jun 9, 2025

View reviewed changes

IvanBorislavovDimitrov previously approved these changes Jun 9, 2025

View reviewed changes

Adopt released cf-java-client

c978648

s-yonkov-yonkov dismissed stale reviews from IvanBorislavovDimitrov and theghost5800 via c978648 June 9, 2025 12:44

theghost5800 approved these changes Jun 9, 2025

View reviewed changes

IvanBorislavovDimitrov approved these changes Jun 9, 2025

View reviewed changes

s-yonkov-yonkov merged commit b89298e into master Jun 9, 2025
7 of 8 checks passed

s-yonkov-yonkov deleted the add-support-for-cnb branch June 9, 2025 12:59

		} else if (dockerInfo != null && lifecycleType != null) {
		throw new SLException("Docker information must not be provided when lifecycle is set to " + lifecycleType + "'.");

		assertEquals("Buildpacks must be provided when lifecycle is set to 'cnb'.", exception.getMessage());
		}

		assertEquals("Buildpacks must not be provided when lifecycle is set to 'docker'.", exception.getMessage());
		}

Add support for cnb (#1629) - LMCROSSITXSADEPLOY-2993 #1630

Add support for cnb (#1629) - LMCROSSITXSADEPLOY-2993 #1630

Uh oh!

Conversation

s-yonkov-yonkov commented May 8, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theghost5800 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud bot commented Jun 9, 2025

Quality Gate passed

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants