Reuse the existing repo as implied one should add it into group#1390
Reuse the existing repo as implied one should add it into group#1390jdcasey merged 4 commits intoCommonjava:masterfrom
Conversation
| { | ||
| logger.debug( "Found existing RemoteRepositories: {}", rrs ); | ||
|
|
||
| for ( final RemoteRepository rr : rrs ) |
There was a problem hiding this comment.
I wouldn't add all of them which is what happens here, right? This does not make much sense. I would find the one with matching releases/snapshots flags and also with empty pathMaskPatterns (which is how new ones are created, right?). If no such repo exists, I would fall back to creating a new one. Does it sound right?
There was a problem hiding this comment.
Yeah, that makes sense, updated.
sswguo
force-pushed
the
implied_existing_repo
branch
from
874a1c1 to
eb9b870
Compare
pkocandr
left a comment
pkocandr
left a comment
There was a problem hiding this comment.
Just minor enhancement could be done, but apart from that it looks ok.
| if ( rrs != null && !rrs.isEmpty() ) | ||
| { | ||
| rrs = rrs.stream() | ||
| .filter( rr -> rr.isAllowReleases() == repo.isReleasesEnabled() ) |
There was a problem hiding this comment.
Wouldn't it be better to compare with the ref instance? It should match, but who knows... :-)
| rrs = rrs.stream() | ||
| .filter( rr -> rr.isAllowReleases() == repo.isReleasesEnabled() ) | ||
| .filter( rr -> rr.isAllowSnapshots() == repo.isSnapshotsEnabled() ) | ||
| .filter( rr -> rr.getPathMaskPatterns() == null || rr.getPathMaskPatterns().isEmpty() ) |
There was a problem hiding this comment.
Also here I would tend to compare the value with the one in ref since we already have it available here.
There was a problem hiding this comment.
Yeah, it's better to add that condition too, to avoid to create a duplicate one with the same pathMaskPatterns.
jdcasey
left a comment
jdcasey
left a comment
There was a problem hiding this comment.
I think we need to be more careful about aggregating with pre-existing metadata / information.
| e ); | ||
| } | ||
|
|
||
| final RemoteRepository ref = creator.createFrom( gav, repo, LoggerFactory.getLogger( creator.getClass() ) ); |
There was a problem hiding this comment.
What's the purpose of this variable? It looks like we're cloning it below, if there are no matching remote repositories (by URL). Is this just to get the creation check done up front?
There was a problem hiding this comment.
That looks so, because we use this variable in several places, except the matching part, but also the following if section and else section. Just want to abstract it here not create it several times.
| rr.setMetadata( METADATA_ORIGIN, IMPLIED_REPO_ORIGIN ); | ||
| try | ||
| { | ||
| rr.setMetadata( IMPLIED_BY_STORES, mapper.writeValueAsString( |
There was a problem hiding this comment.
I think if we're reusing an existing remote repo, we should add the current job key to any existing job keys, so we don't lose the old information (if it's already there).
There was a problem hiding this comment.
Yeah, merging here looks good.
Yeah, that's it. |
3 participants
Patch for https://projects.engineering.redhat.com/browse/NOSSUP-110