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

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).

>> -        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