Merge ubuntu-debuginfod:prepare-getter-for-ppa-support into ubuntu-debuginfod:master

[Sergio Durigan Junior]

This MP implements improvements/adjustments that are needed in the "getter" classes for the upcoming PPA support.

I spent some time trying to come up with a logical way to split the several commits that I have made these past weeks, but they're just too intertwined. So instead, I chose to (try to) split the changes into the "getter" and "poller" areas of the code.

Hopefully this is easy enough to review. As I said in the beginning, this is all prep work; it doesn't introduce any new user-visible feature just yet.

[Lena Voytek (lvoytek)]

The code looks good to me! I added some comments on currently unused variables and a function comment but otherwise I approve

review: Approve

[Sergio Durigan Junior (sergiodj)]

Thanks for the review, Lena.

I addressed your comment about having a docstring for do_process_request. I'll wait until tomorrow before I merge this code in order to give Bryce and Athos a chance to review it, too.

[Bryce Harrington (bryce)]

Thanks, looking now.

Btw, would be good to rename the master branch to 'main' when you get a chance.

[Athos Ribeiro (athos-ribeiro)]

Thanks Sergio!

This LGTM. I added a few comments inline, but they are mostly nitpicks. Feel free to address or ignore them and merge :)

review: Approve

[Sergio Durigan Junior (sergiodj)]

Thanks, Athos. I'm addressing your points now.

[Bryce Harrington (bryce)]

Comments inline.

I think you could safely squash 3f7ceb2 and b9d6063 since they're closely related (their changelog messages are almost the same).

diff --git a/ddebgetter.py b/ddebgetter.py
index 7dc951f..08dcf5e 100644
--- a/ddebgetter.py
+++ b/ddebgetter.py
@@ -30,46 +30,89 @@ from git.exc import GitCommandError
 from debuggetter import DebugGetter, DebugGetterRetry, DEFAULT_MIRROR_DIR
 
 
+def _validate_generic_request(request):
+    """Validate a request coming from Celery.
+
+    :param dict[str, str] request: The dictionary containing the
+        request to be validated."""
+    if request is None:
+        raise TypeError("Invalid request (None)")
+    if request.get("architecture") is None:
+        raise TypeError("No 'architecture' in request")
+
+def validate_ddeb_request(request):
+    """Validate a request to fetch a ddeb from the main archive.
+
+    :param dict[str, str] request: The dictionary containing the
+        request to be validated."""
+    _validate_generic_request(request)
+    if request.get("ddeb_url") is None:
+        raise TypeError("No 'ddeb_url' in request")
+    if request["architecture"] == "source":
+        raise ValueError("Wrong request: source fetch")
+
+def validate_source_code_request(request):
+    """Validate a request to fetch a source package from the main
+    archive.
+
+    :param dict[str, str] request: The dictionary containing the
+        request to be validated."""
+    _validate_generic_request(request)
+    if request.get("source_urls") is None:
+        raise TypeError("No 'source_urls' in request")
+    if request["architecture"] != "source":
+        raise ValueError("Wrong request: ddeb fetch")
+
+
 class DdebGetter(DebugGetter):
     """Get (fetch) a ddeb."""
 
-    def __init__(self, mirror_dir=DEFAULT_MIRROR_DIR):
+    def __init__(self, subdir="ddebs", mirror_dir=DEFAULT_MIRROR_DIR):
         """Initialize the object.
 
         See DebugGetter's __init__ for an explanation of the arguments."""
-        super().__init__(subdir="ddebs", mirror_dir=mirror_dir)
+        super().__init__(subdir=subdir, mirror_dir=mirror_dir)
 
     def process_request(self, request):
         """Process a request, usually coming from Celery.
 
-        :param request: The dictionary containing the information
-            necessary to fetch this ddeb.
-        :type request: dict(str : str)"""
-        if request is None:
-            raise TypeError("Invalid request (None)")
-        if request.get("ddeb_url") is None:
-            raise TypeError("No 'ddeb_url' in request")
-        if request.get("architecture") is None:
-            raise TypeError("No 'architecture' in request")
-        if request["architecture"] == "source":
-            raise ValueError("Wrong request: source fetch")
+        :param dict[str, str] request: The dictionary containing the
+            information necessary to fetch this ddeb."""
+        validate_ddeb_request(request)
+        self.do_process_request(request)
+
+    def do_process_request(self, request, credentials=None):
+        """Perform the actual processing of the request.
 
+        :param dict[str, str] request: The dictionary containing the
+            information necessary to fetch this ddeb.
+
+        :param requests_oauthlib.OAuth1 credentials: The credentials
+            to be used when downloading the artifact from Launchpad.
+            Default is None, which means anonymous."""
         self._logger.debug(f"Processing request to download ddeb: {request}")
         self._download_ddeb(
-            request["source_package"], request["component"], request["ddeb_url"]
+            request["source_package"],
+            request["component"],
+            request["ddeb_url"],
+            credentials=credentials,
         )
 
-    def _download_ddeb(self, source_package, component, ddeb_url):
+    def _download_ddeb(self, source_package, component, ddeb_url, credentials=None):
         """Download a ddeb associated with a package.
 
         :param str source_package: Source package name.
 
         :param str component: Source package component.
 
-        :param str ddeb_url: The ddeb URL."""
+        :param str ddeb_url: The ddeb URL.
+
+        :param requests_oauthlib.OAuth1 credentials: The credentials
+            to be used when downloading the artifact from Launchpad.
+            Default is None, which means anonymous."""
         savepath = self._make_savepath(source_package, component)
         self._logger.debug(f"Downloading '{ddeb_url}' into '{savepath}'")
-        self._download_from_lp(ddeb_url, savepath)
+        self._download_from_lp(ddeb_url, savepath, credentials=credentials)
 
 
 # The function below was taken from git-ubuntu.  We need to make
@@ -106,30 +149,34 @@ def adjust_tar_filepath(tarinfo):
 class DdebSourceCodeGetter(DebugGetter):
     """Get (fetch) the source code associated with a ddeb."""
 
-    def __init__(self, mirror_dir="/srv/debug-mirror"):
-        super().__init__(mirror_dir=mirror_dir, subdir="ddebs")
+    def __init__(self, subdir="ddebs", mirror_dir=DEFAULT_MIRROR_DIR):
+        super().__init__(subdir=subdir, mirror_dir=mirror_dir)
 
     def process_request(self, request):
         """Process a request, usually coming from Celery.
 
-        :param request: The dictionary containing the information
-            necessary to fetch this source code.
-        :type request: dict(str : str)"""
-        if request is None:
-            raise TypeError("Invalid request (None)")
-        if request.get("source_urls") is None:
-            raise TypeError("No 'source_urls' in request")
-        if request.get("architecture") is None:
-            raise TypeError("No 'architecture' in request")
-        if request["architecture"] != "source":
-            raise ValueError("Wrong request: ddeb fetch")
+        :param dict[str, str] request: The dictionary containing the
+            information necessary to fetch this source code."""
+        validate_source_code_request(request)
+        self.do_process_request(request)
+
+    def do_process_request(self, request, fallback_to_git=True, credentials=None):
+        """Perform the actual processing of the request.
+
+        :param dict[str, str] request: The dictionary containing the
+            information necessary to fetch this source code.
 
+        :param requests_oauthlib.OAuth1 credentials: The credentials
+            to be used when downloading the artifact from Launchpad.
+            Default is None, which means anonymous."""
         self._logger.debug(f"Processing request to download source code: {request}")
-        self._process_source(
+        self._download_source_code(
             request["source_package"],
             request["version"],
             request["component"],
             request["source_urls"],
+            fallback_to_git=fallback_to_git,
+            credentials=credentials,
         )
 
     def _download_source_code_from_git(self, source_package, version, filepath):
@@ -181,9 +228,8 @@ class DdebSourceCodeGetter(DebugGetter):
         return True
 
     def _download_source_code_from_dsc(
-        self, source_package, version, filepath, source_urls
+        self, source_package, version, filepath, source_urls, credentials=None
     ):
-
         """Download the source code using the .dsc file.
 
         :param str source_package: Source package name.
@@ -197,13 +243,17 @@ class DdebSourceCodeGetter(DebugGetter):
             package.  This is usually the list returned by the
             sourceFileUrls() Launchpad API call.
 
+        :param requests_oauthlib.OAuth1 credentials: The credentials
+            to be used when downloading the artifact from Launchpad.
+            Default is None, which means anonymous.
+
         :rtype: bool
         This method returns True when the operation succeeds, or False
         *iff* the "dpkg-source -x" command fails to run.  Otherwise,
         this function will throw an exception."""
         with tempfile.TemporaryDirectory(ignore_cleanup_errors=True) as source_dir:
             for url in source_urls:
-                self._download_from_lp(url, source_dir)
+                self._download_from_lp(url, source_dir, credentials=credentials)
 
             dscfile = None
             for f in os.listdir(source_dir):
@@ -245,7 +295,16 @@ class DdebSourceCodeGetter(DebugGetter):
 
             return True
 
-    def _download_source_code(self, source_package, version, component, source_urls):
+    def _download_source_code(
+        self,
+        source_package,
+        version,
+        component,
+        source_urls,
+        fallback_to_git=True,
+        credentials=None,
+    ):
+
         """Download the source code for a package.
 
         :param str source_package: Source package name.
@@ -254,7 +313,15 @@ class DdebSourceCodeGetter(DebugGetter):
 
         :param str component: Source package component.
 
-        :param list source_urls: List of source file URLS."""
+        :param list source_urls: List of source file URLS.
+
+        :param boolean fallback_to_git: Whether we should try to fetch
+            the source code using git if the regular approach (via
+            dget) fails.  Default to True.
+
+        :param requests_oauthlib.OAuth1 credentials: The credentials
+            to be used when downloading the artifact from Launchpad.
+            Default is None, which means anonymous."""
         savepath = self._make_savepath(source_package, component)
         txzfilepath = os.path.join(savepath, f"{source_package}-{version}.tar.xz")
         if os.path.exists(txzfilepath):
@@ -262,14 +329,20 @@ class DdebSourceCodeGetter(DebugGetter):
             return
 
         if self._download_source_code_from_dsc(
-            source_package, version, txzfilepath, source_urls
+            source_package,
+            version,
+            txzfilepath,
+            source_urls,
+            credentials=credentials
         ):
             self._logger.info(
                 f"Downloaded source code from dsc for '{source_package}-{version}' as '{txzfilepath}'"
             )
             return
 
-        if self._download_source_code_from_git(source_package, version, txzfilepath):
+        if fallback_to_git and self._download_source_code_from_git(
+            source_package, version, txzfilepath
+        ):
             self._logger.info(
                 f"Downloaded source code from git for '{source_package}-{version}' as '{txzfilepath}'"
             )
@@ -279,15 +352,3 @@ class DdebSourceCodeGetter(DebugGetter):
         # Launchpad, let's raise an exception signalling that we'd
         # like to retry the task.
         raise DebugGetterRetry()
-
-    def _process_source(self, source_package, version, component, source_urls):
-        """Process the request to fetch the source code.
-
-        :param str source_package: Source package name.
-
-        :param str version: Source package version.
-
-        :param str component: Source package component.
-
-        :param list source_urls: List of source URLs."""
-        self._download_source_code(source_package, version, component, source_urls)
diff --git a/debuggetter.py b/debuggetter.py
index e6b1de0..c96bfc4 100644
--- a/debuggetter.py
+++ b/debuggetter.py
@@ -92,7 +92,7 @@ class DebugGetter:
                 f"Could not create '{dst}' atomically: same file already exists"
             )
 
-    def _download_from_lp(self, url, savepath):
+    def _download_from_lp(self, url, savepath, credentials=None):
         """Download a file from Launchpad.
 
         This method tries to download the file in chunks into a
@@ -102,7 +102,11 @@ class DebugGetter:
         :param str url: The URL that should be downloaded.
 
         :param str savepath: The full path (minus the filename) where
-        the file should be saved."""
+        the file should be saved.
+
+        :param requests_oauthlib.OAuth1 credentials: The credentials
+            to be used when downloading the artifact from Launchpad.
+            Default is None, which means anonymous."""
         filepath = os.path.join(savepath, os.path.basename(parse.urlparse(url).path))
         if os.path.exists(filepath):
             self._logger.debug(f"'{filepath}' exists, doing nothing")
@@ -110,7 +114,9 @@ class DebugGetter:
 
         try:
             with requests.Session() as s:
-                with s.get(url, allow_redirects=True, timeout=10, stream=True) as r:
+                with s.get(
+                    url, allow_redirects=True, timeout=10, stream=True, auth=credentials
+                ) as r:
                     r.raise_for_status()
                     with tempfile.NamedTemporaryFile(mode="wb") as tmpfile:
                         shutil.copyfileobj(r.raw, tmpfile)
@@ -127,3 +133,11 @@ class DebugGetter:
             raise DebugGetterTimeout()
 
         self._logger.debug(f"Saved '{filepath}'")
+
+    @property
+    def subdir(self):
+        return self._subdir
+
+    @subdir.setter
+    def subdir(self, subdir):
+        self._subdir = subdir