-
-
Notifications
You must be signed in to change notification settings - Fork 268
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[INFRA-2562] Add 'Artifactory API' mode (plus considerable overhaul to get there) #364
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reviewed the new Artifactory code, and it looks legit. I also run the code locally, and it looks to be working much faster than the original version. Still much slower than 50s reported in another PR, but I have no mirror nearby :)
Approved assuming that: the new mode is documented, especially "How to enable it?"
I would also suggest to firstly deploy it on the experimental update center so that we can do some testing. Or maybe we could have a temporary update center like we previously did for Java 11
* | ||
* @author Kohsuke Kawaguchi | ||
*/ | ||
public abstract class BaseMavenRepository implements MavenRepository { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be converted to an interface with default methods. It might be more convenient for future extensibility.
Will need getter/setter for pluginFilters
, so might be YAGNI
this.pluginFilters.clear(); | ||
} | ||
|
||
private List<PluginFilter> pluginFilters = new ArrayList<>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move definition up in the class?
import java.util.jar.Manifest; | ||
import java.util.stream.Collectors; | ||
|
||
public class ArtifactoryRepositoryImpl extends BaseMavenRepository { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Switch all messaging to java.util.logging?
I will merge this tomorrow. The only notable change appears to be the removal of |
Deferred:
This rips out most of the "Maven" integration code that's been a mess for a long time.
While use of the Artifactory/Nexus Maven index remains the default, if
ARTIFACTORY_USERNAME
andARTIFACTORY_PASSWORD
are set, we instead use the Artifactory API to list and download artifacts.Some details on the implementation:
MavenRepositoryImpl
into supertypesArtifactoryRepositoryImpl
and choose the appropriate one inDefaultMavenRepositoryBuilder
.ArtifactSource
implementations into their respectiveMavenRepository
implementations due to overlap. Was silly to keep them separate.This removes the dependency that the update center generation had on up to date Nexus Maven indexes. While the additional delay has been annoying for a long time, INFRA-2562 is a recent problem that made it quite a bit worse when trying to quickly push out security fixes.
Behavior changes
As this completely changes how plugin data is obtained, there are changes in behavior. In my testing, I've found the following differences in
update-center.actual.json
:sshd
1.2 (a module) was released as an HPI, so I need to blacklist this, as now it would show up in the list of plugins.