On Tue, 2024-03-19 at 12:44 +0100, Abderrahim Kitouni wrote: > [...], I propose that buildstream don't impose any > special structure on the configuration data (except for the `name` and > `kind` keys). They are treated the same way as source plugins in this > regard, with a SourceMirror.COMMON_CONFIG_KEY member to be passed to > Node.validate_keys() alongside with plugin specific configuration. > > Of course, `kind` would be optional to allow old mirror definitions to > keep working. Those would be handled by a new core SourceMirror plugin > that replicates the old behaviour. > > This then leaves a hole in the proposal: how would the buildstream > core know which aliases the mirror can handle and which URLs the > mirror will use for each alias? > > I would argue that the latter isn't important: buildstream doesn't > need to know the actual URL (especially that the plugin would then > transform anyway).
This seems reasonable. > For that, I would add a new method to SourceMirror > that returns the number of URLs that the mirror can provide for a > given alias. I don't have a proper name for it, so suggestions > welcome. I would find it clearer (and easier to name) if it just > returned a boolean, but the current mirrors can have more than one URL > per alias. > > # Returns the number of URLs the mirror can provide for `alias`, zero if none. > def get_n_mirrors_for_alias(self, alias: str) -> int If the support for multiple URLs is purely for compatibility with existing non-plugin mirror definitions and we think it's not useful with mirror plugins (i.e., an independent mirror plugin instance should be configured instead), I'd rather keep this out of the public plugin API. One option would be to make the above method internal and make the public method return a bool instead. Another option may be for the public method to return a list of alias strings, not sure which variant is a better fit. > Then, translate_url would get an index rather than the url. > > # Return the `mirror_idx`th URL to use for downloading `source_url`. > # Any extra data that needs to be passed to Source implementations > # can be added to the `extra_data` dict. > def translate_url( > self, > alias: str, > source_url: str, > mirror_idx: int, > extra_data: Dict[str, Any], > ) -> str Here I'm leaning towards introducing some kind of translation object as parameter to provide maximum flexibility. Besides making it possible to provide access to the mirror index (or alias substitute URL) only via an internal method or attribute, this would also make it possible to support translation of unaliased URLs in the future, without having to mark `alias` as an optional parameter. Cheers, Jürg
