Add support for cnb (#1629) - LMCROSSITXSADEPLOY-2993#1630
Merged
Conversation
* add CNB type * add validation for cnb * add validation and tests * update lifecycle to be nullable * refactoring * add tests * update validation
s-yonkov-yonkov
force-pushed
the
add-support-for-cnb
branch
from
e86c145 to
629713f
Compare
theghost5800
reviewed
Jun 5, 2025
| try { | ||
| return LifecycleType.valueOf(lifecycleValue.toUpperCase()); | ||
| } catch (IllegalArgumentException e) { | ||
| throw new SLException("Unsupported lifecycle value: " + lifecycleValue); |
Contributor
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| private void validateLifecycleType(LifecycleType lifecycleType, List<String> buildpacks, DockerInfo dockerInfo) { | ||
| if (lifecycleType == LifecycleType.CNB && (buildpacks == null || buildpacks.isEmpty())) { |
Contributor
There was a problem hiding this comment.
Just use CollectionUtils to check for empty/null list.
Comment on lines
+68
to
+80
| 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 + "'."); | ||
| } |
Contributor
There was a problem hiding this comment.
- 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.
| void testValidateLifecycleWithCnbAndValidBuildpacks() { | ||
| parametersList.add(Collections.singletonMap("lifecycle", "cnb")); | ||
| parametersList.add(Collections.singletonMap("buildpacks", List.of("custom-buildpack-url"))); | ||
| assertDoesNotThrow(() -> parser.parse(parametersList)); |
Contributor
There was a problem hiding this comment.
Isn't it better to validate fields of parsed object that includes proper values?
| @Test | ||
| void testValidateLifecycleWithDockerAndValidDockerInfo() { | ||
| parametersList.add(getDockerParams()); | ||
| assertDoesNotThrow(() -> parser.parse(parametersList)); |
| @Test | ||
| void testValidateLifecycleWithBuildpackAndNoBuildpacks() { | ||
| parametersList.add(Collections.singletonMap("lifecycle", "buildpack")); | ||
| assertDoesNotThrow(() -> parser.parse(parametersList)); |
stiv03
suggested changes
Jun 5, 2025
Comment on lines
+61
to
+63
| } catch (IllegalArgumentException e) { | ||
| throw new SLException("Unsupported lifecycle value: " + lifecycleValue); | ||
| } |
Contributor
There was a problem hiding this comment.
Extract the error message as a constant and use MessageFormat.format instead of string concatenation
Comment on lines
+72
to
+77
| 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'."); | ||
| } |
Comment on lines
+78
to
+79
| } else if (dockerInfo != null && lifecycleType != null) { | ||
| throw new SLException("Docker information must not be provided when lifecycle is set to " + lifecycleType + "'."); |
Contributor
There was a problem hiding this comment.
Same here, extract message and use MessageFormat.format
Comment on lines
+39
to
+40
| assertEquals("Buildpacks must be provided when lifecycle is set to 'cnb'.", exception.getMessage()); | ||
| } |
Contributor
There was a problem hiding this comment.
Extract the expected error message
Comment on lines
+54
to
+55
| assertEquals("Buildpacks must not be provided when lifecycle is set to 'docker'.", exception.getMessage()); | ||
| } |
Contributor
There was a problem hiding this comment.
same here, extract the expected error message
Comment on lines
+28
to
+32
| @Test | ||
| void testValidateLifecycleWithCnbAndValidBuildpacks() { | ||
| parametersList.add(Collections.singletonMap("lifecycle", "cnb")); | ||
| parametersList.add(Collections.singletonMap("buildpacks", List.of("custom-buildpack-url"))); | ||
| assertDoesNotThrow(() -> parser.parse(parametersList)); |
Contributor
There was a problem hiding this comment.
extract strings like "buildpacks", "lifecycle" and "cnb" as constants to avoid duplication
Comment on lines
66
to
73
| 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") |
Contributor
There was a problem hiding this comment.
extract repeated strings like "buildpack-1", "command1", "stack1" and others into constants
stiv03
approved these changes
Jun 6, 2025
theghost5800
previously approved these changes
Jun 6, 2025
Contributor
theghost5800
left a comment
theghost5800
left a comment
There was a problem hiding this comment.
Before merge adopt released version of cf-java-client-sap and squash commits into one
| return null; | ||
| } | ||
| try { | ||
| return LifecycleType.valueOf(lifecycleValue.toUpperCase()); |
Contributor
There was a problem hiding this comment.
can you use the method LifecycleType::from ?
IvanBorislavovDimitrov
previously approved these changes
Jun 9, 2025
s-yonkov-yonkov
dismissed stale reviews from IvanBorislavovDimitrov and theghost5800
via
c978648
|
theghost5800
approved these changes
Jun 9, 2025
IvanBorislavovDimitrov
approved these changes
Jun 9, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Add support for CNB
Original PR: #1560