Sb bazel pipeline #71
Conversation
# Conflicts: # detectable/src/main/java/com/synopsys/integration/detectable/detectables/bazel/parse/RuleConverter.java
…tifactExclusionsTest
Black Duck Security ReportMerging #71 into master will not change security risk. |
|
I've incorporated all of the review feedback:
|
| @@ -42,18 +40,12 @@ | |||
| private final Logger logger = LoggerFactory.getLogger(this.getClass()); | |||
| private final ExternalIdFactory externalIdFactory; | |||
| private final MutableDependencyGraph dependencyGraph; | |||
| private File workspaceDir; | |||
|
|
|||
| public BazelCodeLocationBuilder(final ExternalIdFactory externalIdFactory) { | |||
There was a problem hiding this comment.
I'm skeptical you are getting much benefit from this class and BazelExternalId. Why not just parse to an external id and add to a graph?
Instead of:
BazelExternalId externalId = BazelExternalId.fromBazelArtifactString(artifactString, ":");
bazelCodeLocationBuilder.add(externalId)
Do something like
ExternalId externalId = BazelExternalIdParser.Parse(artifactString, ":")
dependencyGraph.add(externalId);
Or even have a BazelDependencyParser if you would have to have a new Dependency there as well.
| @@ -59,6 +62,11 @@ public DetectableResult applicable() { | |||
| if (StringUtils.isBlank(bazelDetectableOptions.getTargetName())) { | |||
| return new PropertyInsufficientDetectableResult(); | |||
| } | |||
| final File workspaceFile = fileFinder.findFile(environment.getDirectory(), WORKSPACE_FILENAME); | |||
There was a problem hiding this comment.
Would it make sense putting the WORKSPACE_FILENAME part in extractable too so that if I provide the target I still get some kind of error?
| logger.debug("Bazel extractAndPublishResults()"); | ||
| public Extraction extract(final File bazelExe, final File workspaceDir, final BazelWorkspace bazelWorkspace, final String bazelTarget, | ||
| final BazelProjectNameGenerator bazelProjectNameGenerator, final String providedBazelDependencyType) { | ||
| logger.debug("Bazel extraction:"); |
There was a problem hiding this comment.
I like the new extractor!
| public class WorkspaceRuleChooser { | ||
|
|
||
| @NotNull | ||
| public WorkspaceRule choose(final WorkspaceRule ruleFromWorkspaceFile, final String providedBazelDependencyType) throws IntegrationException { |
There was a problem hiding this comment.
I would remove this method (and maybe rename deriveDependencyType to 'choose').
| public class BazelExtractorTest { | ||
|
|
||
| @Test | ||
| public void testMavenJar() throws ExecutableRunnerException, IntegrationException { |
There was a problem hiding this comment.
These tests seem extremely large. Can they be broken down into smaller tests?
… an error if it's not found
This is a rewrite of the bazel extractor, which is a lot more flexible than the previous version.
I believe it's now far more likely we'll be able to plug in support for future languages/rules
without big changes to the code.
Because it's a rewrite, the PR diffs probably aren't useful. A logical sequence of classes to look
at might be:
does further processing on the output of the previous step, until a list of
maven coordinates (group:artifact:version) strings is produced.