continuum-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Brett Porter <br...@apache.org>
Subject Re: svn commit: r763464 - in /continuum/trunk: continuum-purge/ continuum-purge/src/main/java/org/apache/continuum/purge/ continuum-purge/src/main/java/org/apache/continuum/purge/executor/ continuum-webapp/src/main/java/org/apache/continuum/web/util/ conti...
Date Thu, 09 Apr 2009 00:21:34 GMT
Hi Jev,

Some comments on this...

On 09/04/2009, at 9:57 AM, jzurbano@apache.org wrote:

> Modified: continuum/trunk/continuum-purge/pom.xml
> URL: http://svn.apache.org/viewvc/continuum/trunk/continuum-purge/pom.xml?rev=763464&r1=763463&r2=763464&view=diff
> =
> =
> =
> =
> =
> =
> =
> =
> ======================================================================
> --- continuum/trunk/continuum-purge/pom.xml (original)
> +++ continuum/trunk/continuum-purge/pom.xml Wed Apr  8 23:57:04 2009
> @@ -86,6 +86,11 @@
>        </exclusion>
>      </exclusions>
>    </dependency>
> +    <dependency>
> +      <groupId>org.slf4j</groupId>
> +      <artifactId>slf4j-log4j12</artifactId>
> +      <scope>runtime</scope>
> +    </dependency>

I find it's best to just put the simple logger in at test scope and  
let the eventual user (in continuum, the webapp and the CLI apps)  
determine the runtime logging framework.

> +    private static final char DELIM = ' ';
>
> +    private Logger logger = LoggerFactory.getLogger( "AuditLog" );

Instead of doing the log message processing in the executor, how about  
moving all the logging activity into an audit log component, similar  
to the one in redback?
> import org.apache.maven.continuum.ContinuumException;
> import  
> org 
> .apache 
> .maven.continuum.builddefinition.BuildDefinitionServiceException;
> import  
> org 
> .apache 
> .maven.continuum.project.builder.ContinuumProjectBuildingResult;
> @@ -58,9 +63,15 @@
>        throws ContinuumException
>    {
>        ContinuumProjectBuildingResult result = null;
> +
> +        String groupId = "";
> +
> +        String artifactId = "";
> +
> +        String resource = "";
>
>        // TODO: remove this part once uploading of an m2 project  
> with modules is supported ( CONTINUUM-1098 )
> -        if ( checkProtocol == false )
> +        if ( ( checkProtocol == false ) || ( ( checkProtocol ==  
> true ) && ( pomUrl.startsWith( FILE_SCHEME ) ) ) )
>        {
>            MavenXpp3Reader m2pomReader = new MavenXpp3Reader();
>
> @@ -80,10 +91,14 @@
>                }
>
>                Model model = m2pomReader.read( new  
> FileReader( filePath ) );
> +
> +                groupId = model.getGroupId();
> +                artifactId = model.getArtifactId();
> +                resource =  groupId + ":" + artifactId;
>
>                List modules = model.getModules();
>
> -                if ( modules != null && modules.size() != 0 )
> +                if ( ( checkProtocol == false ) && ( modules !=  
> null && modules.size() != 0 ) )
>                {
>                    result = new ContinuumProjectBuildingResult();
>                     
> result.addError( ERROR_UPLOADING_M2_PROJECT_WITH_MODULES );
> @@ -102,12 +117,49 @@
>                throw new  
> ContinuumException( ERROR_READING_POM_EXCEPTION_MESSAGE, e );
>            }
>        }
> +        else
> +        {
> +            if ( ( pomUrl.startsWith( "http" ) ) &&  
> ( pomUrl.endsWith( "pom.xml" ) ) )
> +            {
> +                try
> +                {
> +                    URL url = new URL( pomUrl );
> +                    BufferedReader in = new BufferedReader( new  
> InputStreamReader( url.openStream() ) );
> +                    StringBuilder content = new StringBuilder();
> +                    String line = in.readLine();
> +
> +                    while ( line != null )
> +                    {
> +                        content.append( line );
> +                        line = in.readLine();
> +                    }
> +                    in.close();
> +
> +                    if ( content.length() > 0 )
> +                    {
> +                        groupId = getSubString( content.toString(),  
> "<groupId>", "</groupId>" );
> +                        artifactId =  
> getSubString( content.toString(), "<artifactId>", "</artifactId>" );
> +                        resource = groupId + ":" + artifactId;
> +                    }
> +                }
> +                catch ( MalformedURLException e )
> +                {
> +                     
> addActionError( ERROR_READING_POM_EXCEPTION_MESSAGE );
> +                }
> +                catch ( IOException e )
> +                {
> +                    throw new  
> ContinuumException( ERROR_READING_POM_EXCEPTION_MESSAGE, e );
> +                }
> +            }
> +        }
>
>        if ( result == null )
>        {
>            result = getContinuum().addMavenTwoProject( pomUrl,  
> selectedProjectGroup, checkProtocol, scmUseCache,
>                                                        ! 
> this.isNonRecursiveProject(), this.getBuildDefinitionTemplateId() );
>        }
> +
> +        triggerAuditEvent( getPrincipal(),  
> AuditLogConstants.PROJECT, resource,  
> AuditLogConstants.ADD_M2_PROJECT );
>
>        return result;
>    }
> @@ -159,5 +211,16 @@
>    {
>        this.nonRecursiveProject = nonRecursiveProject;
>    }
> +
> +    private String getSubString( String content, String tagStart,  
> String tagEnd )
> +    {
> +        String subString = "";
> +
> +        int start = content.indexOf( tagStart ) + tagStart.length();
> +        int end = content.indexOf( tagEnd );
> +        subString = content.substring( start, end );
> +
> +        return subString;
> +    }
>
> }

Most of this seems an unrelated change?

>
> +
> +        Project proj = getContinuum().getProject( projectId );
> +        triggerAuditEvent( getPrincipal(),  
> AuditLogConstants.PROJECT, proj.getGroupId() + ":" +  
> proj.getArtifactId(), AuditLogConstants.FORCE_BUILD );

Getting the project can be costly, just for logging - can you use  
existing IDs instead?
>
> +	
> +	        Project proj = getContinuum().getProject( projectId );
> +
> +          triggerAuditEvent( getPrincipal(),  
> AuditLogConstants.PROJECT, proj.getGroupId() + ":" +  
> proj.getArtifactId(), AuditLogConstants.CANCEL_BUILD );

as above :)
>
> +
> +        Project proj = getContinuum().getProject( projectId );
> +        triggerAuditEvent( getPrincipal(),  
> AuditLogConstants.PROJECT, proj.getGroupId() + ":" +
> +                           proj.getArtifactId(),  
> AuditLogConstants.REMOVE_PROJECT );

Another one here.
>
> -                    logger.info( "Removing Project with id=" +  
> projectId );
> +                    Project proj =  
> getContinuum().getProject( projectId );
> +                    triggerAuditEvent( getPrincipal(),  
> AuditLogConstants.PROJECT, proj.getGroupId() + ":" +
> +                                       proj.getArtifactId(),  
> AuditLogConstants.REMOVE_PROJECT );

And one more :)
>
> Modified: continuum/trunk/continuum-webapp/src/main/resources/META- 
> INF/plexus/application.xml
> URL: http://svn.apache.org/viewvc/continuum/trunk/continuum-webapp/src/main/resources/META-INF/plexus/application.xml?rev=763464&r1=763463&r2=763464&view=diff
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> = 
> ======================================================================
> --- continuum/trunk/continuum-webapp/src/main/resources/META-INF/ 
> plexus/application.xml (original)
> +++ continuum/trunk/continuum-webapp/src/main/resources/META-INF/ 
> plexus/application.xml Wed Apr  8 23:57:04 2009
> @@ -157,7 +157,7 @@
>          <allowedScheme>http</allowedScheme>
>          <allowedScheme>https</allowedScheme>
>          <allowedScheme>ftp</allowedScheme>
> -          <!-- <allowedScheme>file</allowedScheme> -->
> +          <allowedScheme>file</allowedScheme>
>        </allowedSchemes>
>      </configuration>
>    </component>

Seems to have snuck in?

>
> +
> +  <logger name="AuditLog">
> +    <level value="info" />
> +    <appender-ref ref="continuumAuditlog" />
> +  </logger>

If using a class, you can have a more specific name like the one  
below. Don't forget to add additivity="false"!

>
>  <logger  
> name="org.codehaus.plexus.redback.struts2.action.AuditEvent"  
> additivity="false">
>    <level value="info" />
>
>

Cheers,
Brett


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message