linaro-ci-dashboard

Code review comment for lp:~pfalcon/linaro-ci-dashboard/kernel_chain

kernel_chain
Merge into trunk

Revision history for this message

Milo Casagrande (milo) wrote on 2012-09-21:

Hi Paul!

Some comments follow inline.

On Fri, Sep 21, 2012 at 10:34 AM, Paul Sokolovsky
<email address hidden> wrote:
> https://code.launchpad.net/~pfalcon/linaro-ci-dashboard/kernel_chain/+merge/125646
> Your team Linaro Infrastructure is requested to review the proposed merge of lp:~pfalcon/linaro-ci-dashboard/kernel_chain into lp:linaro-ci-dashboard.
>
> === modified file 'config_template/jenkins-jobs/AndroidLoop.xml'
> --- config_template/jenkins-jobs/AndroidLoop.xml 2012-09-18 15:08:05 +0000
> +++ config_template/jenkins-jobs/AndroidLoop.xml 2012-09-21 08:33:27 +0000
> @@ -178,7 +178,7 @@
> </builders>
> <publishers>
> <hudson.tasks.ArtifactArchiver>
> - <artifacts>build/out/target/*/*/*.img.bz2,build/out/target/*/*/*.tar.bz2,build/out/target/*/*/MD5SUMS,build/out/*.tar.bz2,build/out/*.xml,build/out/*_config,build/out/lava-job-info,build/out/linaro_kernel_build_cmds.sh,build/out/linaro_android_build_cmds.sh,build/out/**/*EULA*</artifacts>
> + <artifacts>build/out/target/*/*/*.img.bz2,build/out/target/*/*/*.tar.bz2,build/out/target/*/*/MD5SUMS,build/out/*.tar.bz2,build/out/*.xml,build/out/*_config,build/out/lava-job-info,build/out/linaro_kernel_build_cmds.sh,build/out/linaro_android_build_cmds.sh,build/out/**/*EULA*,MANIFEST</artifacts>

Is it possible to be more greedy here? Like "*.img.*", so to avoid
file suffix, or there might be other file types that we do not want?
Minor suggestion though...

> @models.permalink
> def get_detail_url(self):
> return 'KernelLoopDetail', (), {'slug': self.name}
> @@ -135,3 +141,17 @@
> @models.permalink
> def get_update_url(self):
> return 'KernelLoopUpdate', (), {'slug': self.name}
> +
> + def process_build_results(self, loop_build):
> + """This method gets called with list of artifact files from a build.
> + Its responsibility is to categorize artifacts, and prepare result data
> + to be passed to downstream loop in chain. If needed, this method can
> + access content of artifacts (not just list of them).
> + """
> + files = loop_build.list_artifacts()
> + kernel_debs = filter(lambda fname: "linux-image" in fname, files)
> + if len(kernel_debs) == 1:
> + return {"kernel_deb_url": loop_build.get_artifact_url(kernel_debs[0])}
> + else:
> + self.log.warning("No single kernel deb found among: %s", files)
> + return {}

This is more a personal thing than anything else: why not having only
a single exit point? Just using a single return statement.

Overall it looks good and clean, maybe a couple of tests wouldn't hurt.
Thanks!

--
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

Hi Paul!

Some comments follow inline.

On Fri, Sep 21, 2012 at 10:34 AM, Paul Sokolovsky
<paul.sokolovsky@linaro.org> wrote:
> https://code.launchpad.net/~pfalcon/linaro-ci-dashboard/kernel_chain/+merge/125646
> Your team Linaro Infrastructure is requested to review the proposed merge of lp:~pfalcon/linaro-ci-dashboard/kernel_chain into lp:linaro-ci-dashboard.
>
> === modified file 'config_template/jenkins-jobs/AndroidLoop.xml'
> --- config_template/jenkins-jobs/AndroidLoop.xml        2012-09-18 15:08:05 +0000
> +++ config_template/jenkins-jobs/AndroidLoop.xml        2012-09-21 08:33:27 +0000
> @@ -178,7 +178,7 @@
>    </builders>
>    <publishers>
>      <hudson.tasks.ArtifactArchiver>
> -      <artifacts>build/out/target/*/*/*.img.bz2,build/out/target/*/*/*.tar.bz2,build/out/target/*/*/MD5SUMS,build/out/*.tar.bz2,build/out/*.xml,build/out/*_config,build/out/lava-job-info,build/out/linaro_kernel_build_cmds.sh,build/out/linaro_android_build_cmds.sh,build/out/**/*EULA*</artifacts>
> +      <artifacts>build/out/target/*/*/*.img.bz2,build/out/target/*/*/*.tar.bz2,build/out/target/*/*/MD5SUMS,build/out/*.tar.bz2,build/out/*.xml,build/out/*_config,build/out/lava-job-info,build/out/linaro_kernel_build_cmds.sh,build/out/linaro_android_build_cmds.sh,build/out/**/*EULA*,MANIFEST</artifacts>

Is it possible to be more greedy here? Like "*.img.*", so to avoid
file suffix, or there might be other file types that we do not want?
Minor suggestion though...

>      @models.permalink
>      def get_detail_url(self):
>          return 'KernelLoopDetail', (), {'slug': self.name}
> @@ -135,3 +141,17 @@
>      @models.permalink
>      def get_update_url(self):
>          return 'KernelLoopUpdate', (), {'slug': self.name}
> +
> +    def process_build_results(self, loop_build):
> +        """This method gets called with list of artifact files from a build.
> +        Its responsibility is to categorize artifacts, and prepare result data
> +        to be passed to downstream loop in chain. If needed, this method can
> +        access content of artifacts (not just list of them).
> +        """
> +        files = loop_build.list_artifacts()
> +        kernel_debs = filter(lambda fname: "linux-image" in fname, files)
> +        if len(kernel_debs) == 1:
> +            return {"kernel_deb_url": loop_build.get_artifact_url(kernel_debs[0])}
> +        else:
> +            self.log.warning("No single kernel deb found among: %s", files)
> +            return {}

This is more a personal thing than anything else: why not having only
a single exit point? Just using a single return statement.

Overall it looks good and clean, maybe a couple of tests wouldn't hurt.
Thanks!

-- 
Milo Casagrande
Infrastructure Engineer
Linaro.org <www.linaro.org> │ Open source software for ARM SoCs

« Back to merge proposal