juergbi commented on code in PR #1890:
URL: https://github.com/apache/buildstream/pull/1890#discussion_r1508812480


##########
src/buildstream/source.py:
##########
@@ -696,14 +699,22 @@ def get_mirror_directory(self) -> str:
 
         return self.__mirror_directory
 
-    def translate_url(self, url: str, *, alias_override: Optional[str] = None, 
primary: bool = True) -> str:
+    def translate_url(
+        self,
+        url: str,
+        *,
+        alias_override: Optional[str] = None,
+        primary: bool = True,
+        extra_data: Optional[Dict[str, Any]] = None,
+    ) -> str:
         """Translates the given url which may be specified with an alias
         into a fully qualified url.
 
         Args:
            url: A URL, which may be using an alias
            alias_override: Optionally, an URI to override the alias with.
            primary: Whether this is the primary URL for the source.
+           extra_data: Additional data provided by :class:`SourceMirror 
<buildstream.sourcemirror.SourceMirror>` (*Since: 2.2*)

Review Comment:
   Shall we provide guidance how a source plugin should deal with this. I would 
say source plugins should fail if the mirror provides a key it doesn't 
understand. Or is this too restrictive? `DownloadableFileSource` in #1895 
currently doesn't fail in that case, though.



##########
src/buildstream/sourcemirror.py:
##########
@@ -0,0 +1,203 @@
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+#
+#  Authors:
+#        Tristan Van Berkom <[email protected]>
+"""
+SourceMirror - Base source mirror class
+=======================================
+The SourceMirror plugin allows one to customize how
+:func:`Source.translate_url() <buildstream.source.Source.translate_url>` will
+behave when looking up mirrors, allowing some additional flexibility in the
+implementation of source mirrors.
+
+
+.. _core_source_mirror_abstract_methods:
+
+Abstract Methods
+----------------
+For loading and configuration purposes, SourceMirrors may optionally implement
+the :func:`Plugin base class Plugin.configure() method 
<buildstream.plugin.Plugin.configure>`
+in order to load any custom configuration in the `config` dictionary.
+
+The remaining :ref:`Plugin base class abstract methods 
<core_plugin_abstract_methods>` are
+not relevant to the SourceMirror plugin object and need not be implemented.
+
+Sources expose the following abstract methods. Unless explicitly mentioned,

Review Comment:
   ```suggestion
   SourceMirrors expose the following abstract methods. Unless explicitly 
mentioned,
   ```



##########
src/buildstream/sourcemirror.py:
##########
@@ -0,0 +1,203 @@
+#
+#  Licensed under the Apache License, Version 2.0 (the "License");
+#  you may not use this file except in compliance with the License.
+#  You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+#  Unless required by applicable law or agreed to in writing, software
+#  distributed under the License is distributed on an "AS IS" BASIS,
+#  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#  See the License for the specific language governing permissions and
+#  limitations under the License.
+#
+#  Authors:
+#        Tristan Van Berkom <[email protected]>
+"""
+SourceMirror - Base source mirror class
+=======================================
+The SourceMirror plugin allows one to customize how
+:func:`Source.translate_url() <buildstream.source.Source.translate_url>` will
+behave when looking up mirrors, allowing some additional flexibility in the
+implementation of source mirrors.
+
+
+.. _core_source_mirror_abstract_methods:
+
+Abstract Methods
+----------------
+For loading and configuration purposes, SourceMirrors may optionally implement
+the :func:`Plugin base class Plugin.configure() method 
<buildstream.plugin.Plugin.configure>`
+in order to load any custom configuration in the `config` dictionary.
+
+The remaining :ref:`Plugin base class abstract methods 
<core_plugin_abstract_methods>` are
+not relevant to the SourceMirror plugin object and need not be implemented.
+
+Sources expose the following abstract methods. Unless explicitly mentioned,
+these methods are mandatory to implement.
+
+* :func:`SourceMirror.translate_url() 
<buildstream.source.SourceMirror.translate_url>`
+ 
+  Produce an appropriate URL for the given URL and alias.
+
+
+Class Reference
+---------------
+"""
+
+from typing import Optional, Dict, List, Any, TYPE_CHECKING
+
+from .node import MappingNode, SequenceNode
+from .plugin import Plugin
+from ._exceptions import BstError, LoadError
+from .exceptions import ErrorDomain, LoadErrorReason
+
+if TYPE_CHECKING:
+
+    # pylint: disable=cyclic-import
+    from ._context import Context
+    from ._project import Project
+
+    # pylint: enable=cyclic-import
+
+
+class SourceMirrorError(BstError):
+    """This exception should be raised by :class:`.SourceMirror` 
implementations
+    to report errors to the user.
+
+    Args:
+       message: The breif error description to report to the user
+       detail: A possibly multiline, more detailed error message
+       reason: An optional machine readable reason string, used for test cases
+
+    *Since: 2.2*
+    """
+
+    def __init__(
+        self, message: str, *, detail: Optional[str] = None, reason: 
Optional[str] = None, temporary: bool = False
+    ):
+        super().__init__(message, detail=detail, domain=ErrorDomain.SOURCE, 
reason=reason)
+
+
+class SourceMirror(Plugin):
+    """SourceMirror()
+
+    Base SourceMirror class.
+
+    All SourceMirror plugins derive from this class, this interface defines how
+    the core will be interacting with SourceMirror plugins.
+
+    *Since: 2.2*
+    """
+
+    # The SourceMirror plugin type is only supported since BuildStream 2.2
+    BST_MIN_VERSION = "2.2"
+
+    def __init__(
+        self,
+        context: "Context",
+        project: "Project",
+        node: MappingNode,
+    ):
+        # Note: the MappingNode passed here is already expanded with
+        #       the project level base variables, so there is no need
+        #       to expand them redundantly here.
+        #
+        node.validate_keys(["name", "kind", "config", "aliases"])
+
+        # Do local base class parsing first
+        name: str = node.get_str("name")
+        self.__aliases: Dict[str, List[str]] = self.__load_aliases(node)
+
+        # Chain up to Plugin
+        super().__init__(name, context, project, node, "source-mirror")
+
+        # Plugin specific parsing
+        config = node.get_mapping("config", default={})
+        self._configure(config)
+
+    ##########################################################
+    #                        Public API                      #
+    ##########################################################
+    def translate_url(
+        self,
+        *,
+        project_name: str,
+        alias: str,
+        alias_url: str,

Review Comment:
   Could it be sensible to make these two alias parameters optional? They are 
currently always set but we could consider supporting URL translation without 
an alias in the future. Making them optional in the signature now may make it 
easier to support this in the future without breaking backward compatibility.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to