linaro-license-protection

Code review comment for lp:~fboudra/linaro-license-protection/bug978711-create-latest-and-headers-symlinks-v2

bug978711-create-latest-and-headers-symlinks-v2
Merge into trunk

Revision history for this message

Deepti B. Kalakeri (deeptik) wrote on 2012-05-01:

> On 1 May 2012 09:40, Deepti B. Kalakeri <email address hidden> wrote:
> > On Mon, Apr 30, 2012 at 9:53 PM, Fathi Boudra
> <email address hidden>wrote:
> >
> >> Fathi Boudra has proposed merging
> >> lp:~fboudra/linaro-license-protection/bug978711-create-latest-and-headers-
> symlinks-v2
> >> into lp:linaro-license-protection.
> >>
> >> Requested reviews:
> >> Linaro Infrastructure (linaro-infrastructure)
> >> Related bugs:
> >> Bug #978711 in Linaro CI: "Create latest and headers symlinks for
> >> published images"
> >> https://bugs.launchpad.net/linaro-ci/+bug/978711
> >>
> >> For more details, see:
> >>
> >> https://code.launchpad.net/~fboudra/linaro-license-protection/bug978711
> -create-latest-and-headers-
> symlinks-v2/+merge/104142<https://code.launchpad.net/%7Efboudra/linaro-
> license-protection/bug978711-create-latest-and-headers-
> symlinks-v2/+merge/104142>
> >>
> >> Create latest and HEADER.html symlinks for Ubuntu images. (LP: #978711)
> >> --
> >>
> >> https://code.launchpad.net/~fboudra/linaro-license-protection/bug978711
> -create-latest-and-headers-
> symlinks-v2/+merge/104142<https://code.launchpad.net/%7Efboudra/linaro-
> license-protection/bug978711-create-latest-and-headers-
> symlinks-v2/+merge/104142>
> >> Your team Linaro Infrastructure is requested to review the proposed merge
> >> of
> >> lp:~fboudra/linaro-license-protection/bug978711-create-latest-and-headers-
> symlinks-v2
> >> into lp:linaro-license-protection.
> >>
> >> === modified file 'scripts/publish_to_snapshots.py'
> >> --- scripts/publish_to_snapshots.py 2012-04-25 11:25:15 +0000
> >> +++ scripts/publish_to_snapshots.py 2012-04-30 16:22:20 +0000
> >> @@ -129,15 +129,22 @@
> >>
> >> return build_dir_path, target_dir_path
> >>
> >> - def create_symlink(self, target_dir_path):
> >> - target_parent_dir = os.path.dirname(target_dir_path)
> >>
> >
> >
> > We can probably leave this as is and use the target_parent_dir path in the
> > symlink_path = os.path.join(...)
> > This will help us to stick to the 80 column rule and also reduce duplicae
> > call to os.path.dirname.
>
> We don't have duplicate call to os.path.dirname. We have only one call
> happening (different code path).

Yeah, true that we don't execute os.path.dirname() twice at any instance.
I only meant to say the call is duplicate code wise
>
> >> - symlink_path = os.path.join(target_parent_dir, "lastSuccessful")
> >> + def create_symlink(self, args, target_dir_path):
> >> + header_path = os.path.join(target_path, "HEADER.html")
> >> + header_symlink_path = os.path.join(target_dir_path, "HEADER.html")
> >> +
> >> + if args.job_type == "android":
> >> + symlink_path = os.path.join(os.path.dirname(target_dir_path),
> >> "lastSuccessful")
> >> + else:
> >> + symlink_path = os.path.join(os.path.dirname(target_dir_path),
> >> "latest")
> >> +
> >> try:
> >> if os.path.islink(symlink_path):
> >> os.unlink(symlink_path)
> >>
> >> + os.symlink(header_path, header_symlink_path)
> >>
> >
> > Do you want to create symlink from /tmp/www/precise/images/nano/1/ to
> > /tmp/www/HEADER.html ??
> > I think we should have a symlink from /tmp/www/precise/images/nano/1/ to
> > /tmp/www/precise/HEADER.html ?
>
> We want to create /path/to/images/nano/1/HEADER.html -> /path/to/HEADER.html
> The content of the file is generic for all snapshots. It only pulls css theme.
>
> > Between the content of the HEADER.html is empty is this going to be copied
> > from somewhere or generate on fly ?
>
> HEADER.html is a static file, we only have to create the symlink and
> assume the file presence.
>
> >> os.symlink(target_dir_path, symlink_path)
> >> - print "The lastSuccessful build is now linked to ",
> >> target_dir_path
> >> + print "The latest build is now linked to ", target_dir_path
> >> return PASS
> >> except Exception, details:
> >> print "Failed to create symlink", symlink_path, ":", details
> >> @@ -157,7 +164,7 @@
> >> if len(lines) != 0:
> >> fd = open(fn, "w+")
> >> for line in lines:
> >> - if not "MANIFEST" in line:
> >> + if not "MANIFEST" or not "HEADER.html" in line:
> >> fd.write(line)
> >> fd.close()
> >> else:
> >> @@ -205,8 +212,11 @@
> >>
> >> self.move_dir_content(build_dir_path, target_dir_path)
> >>
> >> - if args.job_type == "android":
> >> - ret = self.create_symlink(target_dir_path)
> >> + if args.job_type == "android" or\
> >> + args.job_type == "ubuntu-hwpacks" or\
> >> + args.job_type == "ubuntu-images" or\
> >> + args.job_type == "ubuntu-sysroots":
> >> + ret = self.create_symlink(args, target_dir_path)
> >> if ret != PASS:
> >> return ret
> >>
> >>
> >> === modified file 'testing/test_publish_to_snapshots.py'
> >> --- testing/test_publish_to_snapshots.py 2012-04-24 13:08:18 +0000
> >> +++ testing/test_publish_to_snapshots.py 2012-04-30 16:22:20 +0000
> >> @@ -384,7 +384,7 @@
> >> pass
> >>
> >> stdout.seek(0)
> >> - msg = "The lastSuccessful build is now linked to " +
> >> target_dir_path
> >> + msg = "The latest build is now linked to " + target_dir_path
> >> self.assertIn(msg, stdout.read())
> >>
> >> def test_create_manifest_file_option(self):
> >>
> >>
> >>
> >
> >
> > --
> > Thanks and Regards,
> > Deepti
> >
> > https://code.launchpad.net/~fboudra/linaro-license-protection/bug978711
> -create-latest-and-headers-symlinks-v2/+merge/104142
> > You are the owner of lp:~fboudra/linaro-license-protection/bug978711-create-
> latest-and-headers-symlinks-v2.
>
>
>
> --
> Fathi Boudra
> Linaro Release Manager | Validation Project Manager
> Linaro.org | Open source software for ARM SoCs

> On 1 May 2012 09:40, Deepti B. Kalakeri <deepti.kalakeri@linaro.org> wrote:
> > On Mon, Apr 30, 2012 at 9:53 PM, Fathi Boudra
> <fathi.boudra@linaro.org>wrote:
> >
> >> Fathi Boudra has proposed merging
> >> lp:~fboudra/linaro-license-protection/bug978711-create-latest-and-headers-
> symlinks-v2
> >> into lp:linaro-license-protection.
> >>
> >> Requested reviews:
> >>  Linaro Infrastructure (linaro-infrastructure)
> >> Related bugs:
> >>  Bug #978711 in Linaro CI: "Create latest and headers symlinks for
> >> published images"
> >>  https://bugs.launchpad.net/linaro-ci/+bug/978711
> >>
> >> For more details, see:
> >>
> >> https://code.launchpad.net/~fboudra/linaro-license-protection/bug978711
> -create-latest-and-headers-
> symlinks-v2/+merge/104142<https://code.launchpad.net/%7Efboudra/linaro-
> license-protection/bug978711-create-latest-and-headers-
> symlinks-v2/+merge/104142>
> >>
> >> Create latest and HEADER.html symlinks for Ubuntu images. (LP: #978711)
> >> --
> >>
> >> https://code.launchpad.net/~fboudra/linaro-license-protection/bug978711
> -create-latest-and-headers-
> symlinks-v2/+merge/104142<https://code.launchpad.net/%7Efboudra/linaro-
> license-protection/bug978711-create-latest-and-headers-
> symlinks-v2/+merge/104142>
> >> Your team Linaro Infrastructure is requested to review the proposed merge
> >> of
> >> lp:~fboudra/linaro-license-protection/bug978711-create-latest-and-headers-
> symlinks-v2
> >> into lp:linaro-license-protection.
> >>
> >> === modified file 'scripts/publish_to_snapshots.py'
> >> --- scripts/publish_to_snapshots.py     2012-04-25 11:25:15 +0000
> >> +++ scripts/publish_to_snapshots.py     2012-04-30 16:22:20 +0000
> >> @@ -129,15 +129,22 @@
> >>
> >>         return build_dir_path, target_dir_path
> >>
> >> -    def create_symlink(self, target_dir_path):
> >> -        target_parent_dir = os.path.dirname(target_dir_path)
> >>
> >
> >
> > We can probably leave this as is and use the target_parent_dir path in the
> > symlink_path = os.path.join(...)
> > This will help us to stick to the 80 column rule and also reduce duplicae
> > call to os.path.dirname.
> 
> We don't have duplicate call to os.path.dirname. We have only one call
> happening (different code path).

Yeah, true that we don't execute os.path.dirname() twice at any instance.
I only meant to say the call is duplicate code wise
> 
> >> -        symlink_path = os.path.join(target_parent_dir, "lastSuccessful")
> >> +    def create_symlink(self, args, target_dir_path):
> >> +        header_path = os.path.join(target_path, "HEADER.html")
> >> +        header_symlink_path = os.path.join(target_dir_path, "HEADER.html")
> >> +
> >> +        if args.job_type == "android":
> >> +            symlink_path = os.path.join(os.path.dirname(target_dir_path),
> >> "lastSuccessful")
> >> +        else:
> >> +            symlink_path = os.path.join(os.path.dirname(target_dir_path),
> >> "latest")
> >> +
> >>         try:
> >>             if os.path.islink(symlink_path):
> >>                 os.unlink(symlink_path)
> >>
> >> +            os.symlink(header_path, header_symlink_path)
> >>
> >
> > Do you want to create symlink from /tmp/www/precise/images/nano/1/ to
> > /tmp/www/HEADER.html ??
> > I think we should have a symlink from  /tmp/www/precise/images/nano/1/ to
> > /tmp/www/precise/HEADER.html ?
> 
> We want to create /path/to/images/nano/1/HEADER.html -> /path/to/HEADER.html
> The content of the file is generic for all snapshots. It only pulls css theme.
> 
> > Between the content of the HEADER.html is empty is this going to be copied
> > from somewhere or generate on fly ?
> 
> HEADER.html is a static file, we only have to create the symlink and
> assume the file presence.
> 
> >>             os.symlink(target_dir_path, symlink_path)
> >> -            print "The lastSuccessful build is now linked to ",
> >> target_dir_path
> >> +            print "The latest build is now linked to ", target_dir_path
> >>             return PASS
> >>         except Exception, details:
> >>             print "Failed to create symlink", symlink_path, ":", details
> >> @@ -157,7 +164,7 @@
> >>             if len(lines) != 0:
> >>                 fd = open(fn, "w+")
> >>                 for line in lines:
> >> -                    if not "MANIFEST" in line:
> >> +                    if not "MANIFEST" or not "HEADER.html" in line:
> >>                         fd.write(line)
> >>                 fd.close()
> >>             else:
> >> @@ -205,8 +212,11 @@
> >>
> >>             self.move_dir_content(build_dir_path, target_dir_path)
> >>
> >> -            if args.job_type == "android":
> >> -                ret = self.create_symlink(target_dir_path)
> >> +            if args.job_type == "android" or\
> >> +               args.job_type == "ubuntu-hwpacks" or\
> >> +               args.job_type == "ubuntu-images" or\
> >> +               args.job_type == "ubuntu-sysroots":
> >> +                ret = self.create_symlink(args, target_dir_path)
> >>                 if ret != PASS:
> >>                     return ret
> >>
> >>
> >> === modified file 'testing/test_publish_to_snapshots.py'
> >> --- testing/test_publish_to_snapshots.py        2012-04-24 13:08:18 +0000
> >> +++ testing/test_publish_to_snapshots.py        2012-04-30 16:22:20 +0000
> >> @@ -384,7 +384,7 @@
> >>             pass
> >>
> >>         stdout.seek(0)
> >> -        msg = "The lastSuccessful build is now linked to  " +
> >>  target_dir_path
> >> +        msg = "The latest build is now linked to  " +  target_dir_path
> >>         self.assertIn(msg, stdout.read())
> >>
> >>     def test_create_manifest_file_option(self):
> >>
> >>
> >>
> >
> >
> > --
> > Thanks and Regards,
> > Deepti
> >
> > https://code.launchpad.net/~fboudra/linaro-license-protection/bug978711
> -create-latest-and-headers-symlinks-v2/+merge/104142
> > You are the owner of lp:~fboudra/linaro-license-protection/bug978711-create-
> latest-and-headers-symlinks-v2.
> 
> 
> 
> --
> Fathi Boudra
> Linaro Release Manager | Validation Project Manager
> Linaro.org | Open source software for ARM SoCs

« Back to merge proposal