archiva-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joakim Erdfelt <joa...@erdfelt.com>
Subject Re: svn commit: r590858 - in /maven/archiva/trunk/archiva-base: archiva-policies/src/main/java/org/apache/maven/archiva/policies/ archiva-policies/src/test/java/org/apache/maven/archiva/policies/ archiva-proxy/src/main/java/org/apache/maven/archiva/proxy/
Date Wed, 31 Oct 2007 23:46:31 GMT
Brett Porter wrote:
> Hi Joakim,
>
> On 01/11/2007, at 9:47 AM, joakime@apache.org wrote:
>
>> Fixing release / snapshot policies from applying tests on 
>> maven-metadata.xml files.
>
> Was this the addition of the filetype property - or something else as 
> well? Bit hard to find in the change :)

Yes. the filetype property.
Didn't want to pull in all of archiva-repository-layer just to get the 
benefits of metadata detection.
Felt i could just let the proxy handlers determine scope (artifact / 
metadata / supportfile) and pass it down the the policy handlers.

>
>> Switching from boolean return on .applyPolicy() to throwing 
>> exception, to gain better logging of why the transfer failed.
>
> No need to change it now, but I don't really agree with this. I don't 
> agree with using exceptions for non-exceptional conditions for 
> readability reasons - particularly just to pass a string around. IMO, 
> better alternatives:
> 1) just log in the policy check and return true/false as before
> 2) return an object that has a "success" flag and a "reason" string 
> (essentially the same as an exception, but correctly using the return 
> type)

The flaw with that is that only having a boolean doesn't tell us WHY 
something failed.
Also the log didn't make a distinction between config issue, normal 
happy path, informational, and problem path.
Since this is a failure, throw an exception.
Made the code easier to read too.  Lots of uncoding here.

>
>>              getLogger().debug( "Applying [" + key + "] policy with 
>> [" + setting + "]" );
>> -            if ( !policy.applyPolicy( setting, request, localFile ) )
>> +            try
>>              {
>> -                getLogger().debug( "Didn't pass the [" + key + "] 
>> policy." );
>> -                return false;
>> +                policy.applyPolicy( setting, request, localFile );
>> +            }
>> +            catch ( PolicyConfigurationException e )
>> +            {
>> +                getLogger().error( e.getMessage(), e );
>>              }
>>          }
>
> This is going to be very noisy if it's misconfigured - is there 
> anything that can test these on system configuration so we can fail fast?
As for noise, the standard failure reasoning will easily be 
(10*(proxyConnector.size()) more noisy than the configuration errors.

Working on that now, the DefaultArchivaConfiguration.load() mechanism 
has many configuration cleanup / sanity things in it.
Shame you got rid of the ConfigurationCleanup interface a while back. 
would have made this easy to test individually instead of all lumped 
together like it is now.

Work on MRM-549 is proceeding.  Go comment on that other thread about 
SKIP/REJECT.
The change to that config.load() is due to MRM-549, but I'm cleaning up 
a ton of other proxy connector nonsense too.

-- 
- Joakim Erdfelt
  joakim@erdfelt.com
  Open Source Software (OSS) Developer


Mime
View raw message