[NCL-9447] Define tracking-service API#331
Conversation
pkocandr
force-pushed
the
master
branch
6 times, most recently
from
be226e2 to
cc0d561
Compare
rnc
left a comment
rnc
left a comment
There was a problem hiding this comment.
Looks great. A few questions and one tiny comment :-)
|
|
||
| private String project; | ||
|
|
||
| private String name; |
There was a problem hiding this comment.
Can we clarify how the artifact repositories will be laid out - for instance would it be
<project-key [ i.e. deployment e.g. pnc, pnc-devel, etc ]>-<buildType [maven | npm | etc]>-<repoType [ hosted | group(virtual) ]-build-<id>
Maybe repoType isn't required?
Should build type be included?
e.g.
pnc-maven-build-1234
There was a problem hiding this comment.
I think we need to see which one variant is going to be better when we deploy Artifactory. A lot of worker are deployed per repository prefix so we will want to have the prefix capture correct repos.
F.e. pnc-prod-build-* would be a better prefix imo than pnc-prod-maven-build-* cause we would have to specify all package types seperately so that tracking worker is deployed correctly.
There was a problem hiding this comment.
I think the layout should not be defined in this API but rather in the implementation in repository driver.
But to answer the question, I would also prefer including buildType and put in repoType only in case of a collision, e.g. pnc-maven-build-1234 being the hosted repo for uploads while pnc-maven-group-build-1234 is the group for dependency downloads (or pnc-maven-build-group-1234?). But as Jan said it might be needed to optimize it based on Artifactory's capabilities.
| * The identifier of the repository where the artifact is located. | ||
| */ | ||
| @EqualsAndHashCode.Include | ||
| private final RepositoryId repoId; |
There was a problem hiding this comment.
Question - does the artifact need to record the type of the repository e.g. maven, npm, etc? Or should that be part of repository? (if needed at all)...
There was a problem hiding this comment.
AccessChannel is the field that gives you information about the type of repository the artifact comes from. That field is a part of TrackedEntry class.
There was a problem hiding this comment.
Thanks! This is definitely a valid concern. Package type (as it was called in Indy rather than build type) is missing here. I was thinking where to put it and the best place is probably in TrackedArtifact. Another option was RepositoryId where it seems more logical, but RepositoryId is intended to be used also elsewhere, e.g. in batchDelete operation which should end up in repository driver, where the package type is irrelevant. It needs to be available in Track*Request, so TrackedEntry is not the right place. It would be the case if we could easily identify the packageType without sending it in every tracking request which I think is not possible.
Also as Jan mentioned AccessChannel - that can be probably removed, because if it is an NPM or Maven artifact, it is always accessed natively, while generic http downloads are always accessed via proxy. This was introduced when we had only Maven support in Indy. Now it's kind of duplicating the information from packageType.
Does this suggestion sound ok?
There was a problem hiding this comment.
Package type added below. Access channel has been removed from both TrackedEntry and TrackDownloadRequest.
There was a problem hiding this comment.
I removed the PackageType again since we concluded in a separate discussion, that Repo Driver can pull repository metadata from Artifactory and not rely on what is being sent to Tracking Service.
Repository driver needs this information to put it into purl and also into the RepositoryType in TargetRepository. But it can pull the information form Artifactory directly.
Mend Scan ResultsStatus: ✅ No findings detected SCA scan outputSAST scan output |
3 participants
No description provided.