Hi Brett,
my responses are inline:
Brett Porter wrote:
+
+ private String getWagonHint( String protocol, String repositoryId )
+ {
+ // TODO: Implement a better way to get the hint, via
settings.xml or something.
While this effectively is the same, I think it could be simplified by an
actual wagon implementation that takes the http/https definitions and
delegates each method call to a selected wagon impl. That way the server
config is passed in using normal plexus configuration:
WagonDelegate implements Wagon, Contextualizable {
private String wagonImpl;
private Wagon delegate;
contextualize() {
delegate = container.lookup( ROLE, wagonImpl );
}
put( ... ) {
delegate.put( ... );
}
}
WDYT?
This would also avoid the ordering kludge you needed to add to the POM
later.
Actually, I disagree. First, since the configurator attempts to traverse
the PlexusConfiguration and reflectively assign those values to bean
properties (or fields) in the wagon instance, this means that a
Wagon.class pass-through will have to be able to somehow accommodate all
possible configuration parameters that any delegate would use. While
we might be able to accomplish this by making WagonDelegate implement
MapOrientedComponent, we'd still have to take those mapped variables and
immediately start over again with a new Configurator instance
internally, so we could actually pass on the configuration to the delegate.
Simply resolving the hint and looking up the correct wagon before
configuration takes place makes all of this much, much simpler.
As for the POM ordering issue, we can avoid that simply by providing a
defaultRoleHints map configuration for the wagon manager and/or the
delegator class that looks up the wagon (initially, i had factored out
the Wagon-lookup functionality, but folded it back into WagonManager in
the end). Since maven-core redefines the lw and httpclient http wagon
components using things like 'http-sun', we could configure the wagon
manager to default over to 'http-sun' if no sysprop is provided, then if
the defaultRoleHints mapping doesn't contain an entry for the protocol
in question WagonManager could lookup by the protocol itself.
I'd prefer the SysProp be checked in DefaultMaven (via the execution
properties), like maven.artifact.threads, and then a method called on
the wagon manager to set a default wagon impl (when registerWagons is
called).
I don't have any serious issue with moving the sysprop check; I just
figured it made more sense architecturally to have all of the
configuration logic for wagons in one place (as much as the api
separation allows).
+ for ( Iterator<String> i = wagons.iterator(); i.hasNext(); )
can change to:
for ( String protocol : wagons ) { availableWagons.put( protocol,
extensionContainer ); }
yeah, that got left out while I was trying to debug a problem I was
having, and I lost track of it.
Is it possible wagons will be null if the prior use case is encountered,
or will it return an empty map?
I'll fix that in my next commit.
-john
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
