Split build-info class #559
Conversation
| @@ -81,40 +88,39 @@ public Build build() { | |||
| throw new IllegalArgumentException("Build start time must be set"); | |||
| } | |||
|
|
|||
| Build build = new Build(); | |||
| BuildInfo buildInfo = new BuildInfo(); | |||
| if (StringUtils.isNotBlank(version)) { | |||
There was a problem hiding this comment.
Is this if statement needed? Seems redundant.
There was a problem hiding this comment.
The If makes sure that the default version in new BuildInfo(); won't get an override.
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildInfoBuilder.java
Outdated
Show resolved
Hide resolved
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildInfoBuilder.java
Outdated
Show resolved
Hide resolved
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildInfoBuilder.java
Outdated
Show resolved
Hide resolved
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildBuilder.java
Outdated
Show resolved
Hide resolved
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildBuilder.java
Outdated
Show resolved
Hide resolved
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildBuilder.java
Outdated
Show resolved
Hide resolved
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildBuilder.java
Outdated
Show resolved
Hide resolved
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildBuilder.java
Outdated
Show resolved
Hide resolved
build-info-api/src/main/java/org/jfrog/build/api/builder/BuildBuilder.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I haven't finished going over everything, but I suggest you go ahead and review my comments so far, comment or modify the code if needed. Once you're done, I'll resume my review.
This is because I'm mostly concerned about the usage of "ci" classes by "api" classes. See my inline comments and let me know what you think.
| import java.util.Properties; | ||
|
|
||
|
|
||
| public class BuildInfoModuleBuilder { |
There was a problem hiding this comment.
It's not clear what the relationship between this class and the BuildModuleBuilder which exists in the same package. Should what of them be removed?
There was a problem hiding this comment.
BuildModuleBuilder - build the old 'build' class (used by Artifactory)
BuildInfoModuleBuilder - build the new 'build' class (used by Jenkins, etc..)
Until now, Build-info-java has one class that includes all the build-info schema properties & extra logic. Examples of build-info logic are recording artifacts/dependencies local path or append modules/builds.
The one-class structure needs to be splitted into two classes. The first class would have only build-info properties that Artifactory will be using, and the second would have all the extra logic + properties which will be used by other consumers (Jenkins).
Relates PR: