abderrahim commented on code in PR #87:
URL:
https://github.com/apache/buildstream-plugins/pull/87#discussion_r2068670615
##########
src/buildstream_plugins/sources/docker.py:
##########
@@ -57,13 +57,44 @@
#
# **Since**: 2.0.1
+ # Specify the version to be reported as the *guess_version* when reporting
+ # SourceInfo
+ #
+ # Since: 2.5
+ #
+ version: 1.2
+
Note that Docker images may contain device nodes. BuildStream elements cannot
contain device nodes so those will be dropped. Any regular files in the /dev
directory will also be dropped.
See `built-in functionality doumentation
<https://docs.buildstream.build/master/buildstream.source.html#core-source-builtins>`_
for
details on common configuration options for sources.
+
+
+Reporting `SourceInfo
<https://docs.buildstream.build/master/buildstream.source.html#buildstream.source.SourceInfo>`_
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The docker source reports the URL of the docker registry as the *url*.
+
+Further, the docker source reports the ``SourceInfoMedium.OCI_IMAGE`` *medium*
and
+the ``SourceVersionType.DIGEST`` *version_type*, for which it reports the
content
+digest of the docker image as the *version*.
+
+Additionally, the docker source reports the docker image name through
+the ``image-name`` key of the *extra_data*.
+
+As such, after removing the scheme from the URL (i.e. remove ``https://``) the
same docker
Review Comment:
I think it makes more sense to have `docker://` as a scheme rather than
`https://`. Docker itself would accept neither, but podman would accept
`docker://` and it would make sense to ask people to remove `docker://` from a
url before passing it to docker.
##########
src/buildstream_plugins/sources/git.py:
##########
@@ -158,9 +166,33 @@
- `ref-not-in-track
<https://docs.buildstream.build/master/buildstream.types.html#buildstream.types.CoreWarnings.REF_NOT_IN_TRACK>`_
-
The provided ref was not found in the provided track in the element's git
repository.
-"""
+Reporting `SourceInfo
<https://docs.buildstream.build/master/buildstream.source.html#buildstream.source.SourceInfo>`_
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The git source reports the URL of the git repository as the *url*.
+
+Further, the git source reports the ``SourceInfoMedium.GIT`` *medium* and
+the ``SourceVersionType.COMMIT`` *version_type*, for which it reports the git
+commit sha as the *version*.
+
+Since the git source does not have a way to know what the release version
+corresponds to the commit sha, the git source exposes the ``version``
configuration
+attribute to allow explicit specification of the *guess_version*, for the
toplevel
Review Comment:
I think it makes sense to support guessing version from the tag name in the
case of a `git-describe` format ref. But since this plugin is mostly unused,
I'm not really opposed to skipping that.
##########
src/buildstream_plugins/sources/docker.py:
##########
@@ -57,13 +57,44 @@
#
# **Since**: 2.0.1
+ # Specify the version to be reported as the *guess_version* when reporting
+ # SourceInfo
+ #
+ # Since: 2.5
+ #
+ version: 1.2
+
Note that Docker images may contain device nodes. BuildStream elements cannot
contain device nodes so those will be dropped. Any regular files in the /dev
directory will also be dropped.
See `built-in functionality doumentation
<https://docs.buildstream.build/master/buildstream.source.html#core-source-builtins>`_
for
details on common configuration options for sources.
+
+
+Reporting `SourceInfo
<https://docs.buildstream.build/master/buildstream.source.html#buildstream.source.SourceInfo>`_
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The docker source reports the URL of the docker registry as the *url*.
+
+Further, the docker source reports the ``SourceInfoMedium.OCI_IMAGE`` *medium*
and
+the ``SourceVersionType.DIGEST`` *version_type*, for which it reports the
content
Review Comment:
I'm not comfortable overloading the meaning of `SourceVersionType.DIGEST`
like this. BuildStream core uses it for a CAS digest, and the docker digest is
pretty different. I almost feel that `SHA256` would be more appropriate, though
having the `sha256:` prefix makes it different from how the core uses `SHA256`.
##########
src/buildstream_plugins/sources/bzr.py:
##########
@@ -53,17 +53,37 @@
# revision number to the one on the tip of the branch specified in 'track'.
ref: 6622
+ # Specify the version to be reported as the *guess_version* when reporting
+ # SourceInfo
+ #
+ # Since 2.5
+ #
+ version: 1.2
+
See `built-in functionality doumentation
<https://docs.buildstream.build/master/buildstream.source.html#core-source-builtins>`_
for
details on common configuration options for sources.
+
+
+Reporting `SourceInfo
<https://docs.buildstream.build/master/buildstream.source.html#buildstream.source.SourceInfo>`_
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The bzr source reports the URL of the bzr repository as the *url*.
+
+Further, the bzr source reports the ``SourceInfoMedium.BZR`` *medium* and
+the ``SourceVersionType.COMMIT`` *version_type*, for which it reports the bzr
revision
+number as the *version*.
+
+Since the bzr source does not have a way to know what the release version
+corresponds to the revision number, the bzr source exposes the ``version``
configuration
+attribute to allow explicit specification of the *guess_version*.
"""
import os
import shutil
import fcntl
from contextlib import contextmanager
-from buildstream import Source, SourceError
+from buildstream import Source, SourceError, SourceInfoMedium,
SourceVersionType
Review Comment:
I'm a bit worried that this causes the plugin to not be loadable in
buildstream < 2.5. Would it make sense to delay the imports until
`collect_source_info()` is called?
If you think it's better to hard-depend on buildstream 2.5, then we should
update the minimum version (at least in `project.conf`, I don't see an explicit
dependency on buildstream on the python side)
##########
src/buildstream_plugins/sources/git.py:
##########
@@ -986,6 +1027,25 @@ def validate_cache(self):
warning_token=CoreWarnings.REF_NOT_IN_TRACK,
)
+ def collect_source_info(self):
+ #
+ # Currently we cannot implement version guessing, because we do not
save any tag
+ # information in the ref at tracking time.
+ #
+ # Also we do *not* support reporting on submodules, anyway as the
toplevel
+ # git repo determines the git commits of submodules, we consider the
toplevel
+ # git repo to be a comprehensive version of the overall input.
+ #
+ return [
+ self.create_source_info(
+ self.mirror.url,
+ SourceInfoMedium.GIT,
+ SourceVersionType.COMMIT,
+ self.mirror.ref,
Review Comment:
The documentation says a commit sha, but here you're returning the ref
as-is. It could be a git describe formatted ref. Should we do something about
it? (maybe just update the docs to point this out)
##########
src/buildstream_plugins/sources/docker.py:
##########
@@ -57,13 +57,44 @@
#
# **Since**: 2.0.1
+ # Specify the version to be reported as the *guess_version* when reporting
+ # SourceInfo
+ #
+ # Since: 2.5
+ #
+ version: 1.2
+
Note that Docker images may contain device nodes. BuildStream elements cannot
contain device nodes so those will be dropped. Any regular files in the /dev
directory will also be dropped.
See `built-in functionality doumentation
<https://docs.buildstream.build/master/buildstream.source.html#core-source-builtins>`_
for
details on common configuration options for sources.
+
+
+Reporting `SourceInfo
<https://docs.buildstream.build/master/buildstream.source.html#buildstream.source.SourceInfo>`_
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+The docker source reports the URL of the docker registry as the *url*.
+
+Further, the docker source reports the ``SourceInfoMedium.OCI_IMAGE`` *medium*
and
+the ``SourceVersionType.DIGEST`` *version_type*, for which it reports the
content
+digest of the docker image as the *version*.
+
+Additionally, the docker source reports the docker image name through
+the ``image-name`` key of the *extra_data*.
+
+As such, after removing the scheme from the URL (i.e. remove ``https://``) the
same docker
+image can be obtained by calling:
+
+.. code:: bash
+
+ docker pull <url-without-scheme>/<image-name>@<version>
Review Comment:
Is there a reason to report the url and the image separately? Wouldn't it
make more sense to report everything as the url and avoid extra-data?
--
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: commits-unsubscr...@buildstream.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
