launchpad-reviewers team mailing list archive

[Guruprasad <mp+480435@xxxxxxxxxxxxxxxxxx>]

Diff comments:

> diff --git a/lib/lp/archivepublisher/scripts/base.py b/lib/lp/archivepublisher/scripts/base.py
> index f590791..757c909 100644
> --- a/lib/lp/archivepublisher/scripts/base.py
> +++ b/lib/lp/archivepublisher/scripts/base.py
> @@ -36,6 +37,34 @@ class PublisherScript(LaunchpadCronScript):
>              help="Publish all Ubuntu-derived distributions.",
>          )
>  
> +    def addParallelPublisherOptions(self):
> +        self.parser.add_option(
> +            "--archive",
> +            action="append",
> +            dest="archives",
> +            metavar="REFERENCE",
> +            default=[],
> +            help="Only run over the archives identified by this reference. "
> +            "You can specify multiple archives by repeating the option",
> +        )
> +
> +        self.parser.add_option(
> +            "--exclude",
> +            action="append",
> +            dest="excluded_archives",
> +            metavar="REFERENCE",
> +            default=[],
> +            help="Skip the archives identified by this reference in the "
> +            "publisher run. You can specify multiple archives by repeating "
> +            "the option",
> +        )
> +
> +        self.parser.add_option(
> +            "--lockfilename",
> +            dest="lockfilename",

'lock_file_name' or 'lockfile_name'?

> +            help="lockfilename to be used by the publisher run",

"Lock file name"

> +        )
> +
>      def findSelectedDistro(self):
>          """Find the `Distribution` named by the --distribution option.
>  
> @@ -62,3 +91,35 @@ class PublisherScript(LaunchpadCronScript):
>              return self.findDerivedDistros()
>          else:
>              return [self.findSelectedDistro()]
> +
> +    def findArchives(self, archive_references, distribution=None):
> +        """
> +        Retrieve a list of archives based on the provided references and
> +        optional distribution.
> +
> +        Args:
> +            archive_references (list): A list of archive references to
> +            retrieve.
> +            distribution (IDistributionSet, optional): The distribution
> +            to filter archives by. Defaults to None.
> +
> +        Returns:
> +            list: A list of archives that match the provided references and
> +            distribution.
> +        """
> +        if not archive_references:
> +            return []
> +
> +        archives = []
> +        for reference in archive_references:
> +            archive = getUtility(IArchiveSet).getByReference(reference)
> +            if not archive:
> +                self.logger.warning(
> +                    "Cannot find the excluded archive with reference: '%s', "
> +                    % reference
> +                )
> +                continue
> +            if distribution and archive.distribution != distribution:
> +                continue

Should we log a warning or do something here when this condition is true?

> +            archives.append(archive)
> +        return archives
> diff --git a/lib/lp/archivepublisher/scripts/publishdistro.py b/lib/lp/archivepublisher/scripts/publishdistro.py
> index 38ee406..22cc478 100644
> --- a/lib/lp/archivepublisher/scripts/publishdistro.py
> +++ b/lib/lp/archivepublisher/scripts/publishdistro.py
> @@ -612,28 +622,69 @@ class PublishDistro(PublisherScript):
>              ),
>              "--delete",
>              "--delete-after",
> -            rsync_src,
> -            rsync_dest,
>          ]
> -        try:
> -            self.logger.info(
> -                "Attempting to rsync the OVAL data from '%s' to '%s'",
> -                rsync_src,
> -                rsync_dest,
> -            )
> -            check_call(rsync_command)
> -        except CalledProcessError:
> -            self.logger.exception(
> -                "Failed to rsync OVAL data from '%s' to '%s'",
> -                rsync_src,
> -                rsync_dest,
> +        rsync_command.extend(extra_options)
> +        rsync_command.extend([rsync_src, rsync_dest])
> +        return rsync_command
> +
> +    def _generateOVALDataRsyncCommands(self):
> +        if self.options.archives:
> +            return [
> +                self._buildRsyncCommand(
> +                    # -R: copies the OVALData and preserves the src path in
> +                    # dest directory
> +                    # --ignore-missing-args: If the source directory is not
> +                    # present, don't throw an error
> +                    # man rsync can provide detailed explanation of these
> +                    # options
> +                    extra_options=["-R", "--ignore-missing-args"],
> +                    src=os.path.join(
> +                        config.archivepublisher.oval_data_rsync_endpoint,
> +                        reference,
> +                    ),
> +                    dest=os.path.join(config.archivepublisher.oval_data_root),
> +                )
> +                for reference in self.options.archives
> +            ]
> +
> +        exclude_options = []
> +        # If there are any archives specified to be excluded, exclude rsync
> +        # for them in the rsync command
> +        for excluded_archive in self.findArchives(
> +            self.options.excluded_archives
> +        ):
> +            exclude_options.append("--exclude")
> +            exclude_options.append(excluded_archive.reference)

Can you change this to .extend() and include both related options above in the same call so that it is obvious that they need to go together?

> +        return [
> +            self._buildRsyncCommand(
> +                extra_options=exclude_options,
> +                src=config.archivepublisher.oval_data_rsync_endpoint,
> +                dest=config.archivepublisher.oval_data_root,
>              )
> -            raise
> +        ]
> +
> +    def rsyncOVALData(self):
> +        for rsync_command in self._generateOVALDataRsyncCommands():
> +            try:
> +                self.logger.info(
> +                    "Attempting to rsync the OVAL data: %s",
> +                    rsync_command,
> +                )
> +                check_call(rsync_command)
> +            except CalledProcessError:
> +                self.logger.exception(
> +                    "Failed to rsync OVAL data: %s",
> +                    rsync_command,
> +                )
> +                raise
>  
>      def checkForUpdatedOVALData(self, distribution):
>          """Compare the published OVAL files with the incoming one."""
>          start_dir = Path(config.archivepublisher.oval_data_root)
>          archive_set = getUtility(IArchiveSet)
> +        excluded_archives = self.findArchives(
> +            self.options.excluded_archives, distribution
> +        )

This appears to be unused. Please check and remove if it is not needed.

>          for owner_path in start_dir.iterdir():
>              if not owner_path.name.startswith("~"):
>                  continue


-- 
https://code.launchpad.net/~tushar5526/launchpad/+git/launchpad/+merge/480435
Your team Launchpad code reviewers is requested to review the proposed merge of ~tushar5526/launchpad:add-support-for-parallel-publishing into launchpad:master.