[PATCH] repos: Add tags property to repos alongside commit, branch and deprecated refspec

[fe...@piedallu.me]

Hello,
It occurred to me the `refspec` property is being deprecated and splitted between `commit`and `branch`.
That's fine but we are now missing a way to provide a `tag`.
These patches try to add back this feature.
It's easier to handle than `branch` because tags are not namespaced by remote, so it's closer to `commit`.
(Please be gentle, it's my first time sending patches to a mailing list o/)

Félix Piédallu

[Jan Kiszka]

Hi Félix,

Thanks for bringing this up. Unfortunately, it's hard to comment on your
patches as you didn't send them inline (git send-email may help). But
let's look at the use case first (arguing about that would belong into
the commit message(s)):

For now, kas supports two types of repo revision references. One is the
stable SHA (commit), the other is the floating branch. This covers the
two use cases that you either want to pin down a repo or track a latest
version of it for CI purposes. Now, a tag does not fall into any of
these scenarios. It's normally not floating but it is also not stable
(only a commit is). Why do we still want it then? And how do we avoid
that users falsely consider a tag stable? Same reasoning as for bitbake
here.

Jan

--
Siemens AG, Technology
Linux Expert Center

[fe...@piedallu.me]

Hi Jan,

> Unfortunately, it's hard to comment on your patches as you didn't
> send them inline (git send-email may help)

Ah, sorry. My org blocks SMTP so git send-email wasn’t usable.
I pasted my patches in this email.

> Now, a tag does not fall into any of these scenarios. It's normally
> not floating but it is also not stable (only a commit is). Why do we
> still want it then?

The reason we want tags instead of SHAs is that SHAs are not readable.
You can’t tell easily what "release" it’s pointing. That’s what tags are for :)

I fully agree tags are not stable, but they should be expected to be so,
*as long as you trust the repository* of course. That is the case for
organisation internal repositories, where tags are generaly used.

Also, I’m working with multi-repo orgs that create the same tag on all the
repositories ; effectively allowing devs to ensure all repos are in sync in
their local copy or Kas config files.


That said, I have an idea about how we could make tags more trustworthy:

* If the repo only declares `tag`: this isn’t trustworthy and might trigger a
runtime warning;
* If the repo declares `tag` and `commit`: Kas should check that the given
`tag` resolves to the given `commit` and, if not, should *error* out.

This strategy could ensure trustworthy *and* readable repo declarations.

I believe the current behaviour implemented by my patches is to fail if both
`commit` and `tag` are given, just like `commit` and `branch`.


Would you consider this ?

Thanks for your reply,

Félix Piédallu

8 août 2023 08:28 "Jan Kiszka' via kas-devel" <kas-...@googlegroups.com> a écrit:

> Hi Félix,

>
> Thanks for bringing this up. Unfortunately, it's hard to comment on your
> patches as you didn't send them inline (git send-email may help). But
> let's look at the use case first (arguing about that would belong into
> the commit message(s)):
>
> For now, kas supports two types of repo revision references. One is the
> stable SHA (commit), the other is the floating branch. This covers the
> two use cases that you either want to pin down a repo or track a latest
> version of it for CI purposes. Now, a tag does not fall into any of
> these scenarios. It's normally not floating but it is also not stable
> (only a commit is). Why do we still want it then? And how do we avoid
> that users falsely consider a tag stable? Same reasoning as for bitbake
> here.
>
> Jan
>
> --
> Siemens AG, Technology
> Linux Expert Center
>

> --
> You received this message because you are subscribed to the Google Groups "kas-devel" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to
> kas-devel+...@googlegroups.com.
> To view this discussion on the web visit
> https://groups.google.com/d/msgid/kas-devel/59016610-5451-c79b...@siemens.com.

From 045bdf42126b1e04f52868ee56847699c1671978 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix=20Pi=C3=A9dallu?= <fe...@piedallu.me>
Date: Fri, 28 Jul 2023 09:58:43 +0200
Subject: [PATCH 1/2] Add tags property to repos alongside commit, branch and
deprecated refspec
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Félix Piédallu <fe...@piedallu.me>
---
docs/userguide.rst | 4 ++
kas/libkas.py | 4 +-
kas/plugins/dump.py | 8 ++--
kas/plugins/for_all_repos.py | 5 +++
kas/repos.py | 72 +++++++++++++++++++++---------------
kas/schema-kas.json | 3 ++
6 files changed, 61 insertions(+), 35 deletions(-)

diff --git a/docs/userguide.rst b/docs/userguide.rst
index ae1b56d..405946d 100644
--- a/docs/userguide.rst
+++ b/docs/userguide.rst
@@ -443,6 +443,10 @@ Configuration reference
The upstream branch that should be tracked. If no ``commit`` was
specified, the head of the upstream is checked out.

+ * ``tag``: string [optional]
+ The tag that should be used. If no ``commit`` was
+ specified, the commit referenced by this tag is checked out.
+
* ``path``: string [optional]
The path where the repository is stored.
If the ``url`` and ``path`` is missing, the repository where the
diff --git a/kas/libkas.py b/kas/libkas.py
index 613b38a..ac76136 100644
--- a/kas/libkas.py
+++ b/kas/libkas.py
@@ -412,8 +412,8 @@ def setup_parser_common_args(parser):
help='Skip build steps',
default=[])
parser.add_argument('--force-checkout', action='store_true',
- help='Always checkout the desired commit/branch of '
- 'each repository, discarding any local changes')
+ help='Always checkout the desired commit/branch/tag '
+ 'of each repository, discarding any local changes')
parser.add_argument('--update', action='store_true',
help='Pull new upstream changes to the desired '
'branch even if it is already checked out locally')
diff --git a/kas/plugins/dump.py b/kas/plugins/dump.py
index 8d4a990..3588f5b 100644
--- a/kas/plugins/dump.py
+++ b/kas/plugins/dump.py
@@ -31,9 +31,9 @@

When running with ``--lock``, a locking spec is created which only contains
the exact commit of each repository. This can be used to pin the commit of
- floating branches, while still keeping an easy update path. When combining
- with ``--inplace``, a lockfile is created next to the first file on the kas
- cmdline. For details on the locking support, see
+ floating branches and tags, while still keeping an easy update path. When
+ combining with ``--inplace``, a lockfile is created next to the first file
+ on the kas cmdline. For details on the locking support, see
:class:`kas.includehandler.IncludeHandler`.

Please note:
@@ -208,7 +208,7 @@ class Dump(Checkout):

if args.resolve_refs and not args.lock:
for r in repos:
- if r.commit or r.branch:
+ if r.commit or r.branch or r.tag:
config_expanded['repos'][r.name]['commit'] = r.revision
elif r.refspec:
config_expanded['repos'][r.name]['refspec'] = r.revision
diff --git a/kas/plugins/for_all_repos.py b/kas/plugins/for_all_repos.py
index b720eb2..d763f93 100644
--- a/kas/plugins/for_all_repos.py
+++ b/kas/plugins/for_all_repos.py
@@ -56,6 +56,10 @@
repository, or an empty string if no branch was given in the config
file.

+ * ``KAS_REPO_TAG``: The tag which was checked out for this
+ repository, or an empty string if no branch was given in the config
+ file.
+
* ``KAS_REPO_REFSPEC``: The refspec which was checked out for this
repository, or an empty string if no refspec was given in the config
file. This variable is obsolete and will be removed when support for
@@ -119,6 +123,7 @@ class ForAllReposCommand(Command):
'KAS_REPO_URL': '' if repo.operations_disabled else repo.url,
'KAS_REPO_COMMIT': repo.commit or '',
'KAS_REPO_BRANCH': repo.branch or '',
+ 'KAS_REPO_TAG': repo.tag or '',
'KAS_REPO_REFSPEC': repo.refspec or '',
}
logging.info('%s$ %s', repo.path, self.command)
diff --git a/kas/repos.py b/kas/repos.py
index 95038c1..9697eab 100644
--- a/kas/repos.py
+++ b/kas/repos.py
@@ -76,12 +76,13 @@ class Repo:
Represents a repository in the kas configuration.
"""

- def __init__(self, name, url, path, commit, branch, refspec, layers,
+ def __init__(self, name, url, path, commit, tag, branch, refspec, layers,
patches, disable_operations):
self.name = name
self.url = url
self.path = path
self.commit = commit
+ self.tag = tag
self.branch = branch
self.refspec = refspec
self._layers = layers
@@ -113,6 +114,8 @@ class Repo:
elif item == 'revision':
if self.commit:
return self.commit
+ if self.tag:
+ return self.tag
branch = self.branch or self.refspec
if not branch:
return None
@@ -126,11 +129,13 @@ class Repo:
raise AttributeError

def __str__(self):
- if self.commit and self.branch:
- return '%s:%s(%s) %s %s' % (self.url, self.commit, self.branch,
+ if self.commit and (self.tag or self.branch):
+ return '%s:%s(%s) %s %s' % (self.url, self.commit,
+ self.tag or self.branch,
self.path, self._layers)
return '%s:%s %s %s' % (self.url,
- self.commit or self.branch or self.refspec,
+ self.commit or self.tag or self.branch
+ or self.refspec,
self.path, self._layers)

__legacy_refspec_warned__ = []
@@ -168,25 +173,26 @@ class Repo:
name = repo_config.get('name', name)
typ = repo_config.get('type', 'git')
commit = repo_config.get('commit', None)
+ tag = repo_config.get('tag', None)
branch = repo_config.get('branch', repo_defaults.get('branch', None))
refspec = repo_config.get('refspec',
repo_defaults.get('refspec', None))
- if commit is None and branch is None and refspec is None \
- and url is not None:
- raise RepoRefError('No commit or branch specified for repository '
- '"{}". This is only allowed for local '
- 'repositories.'.format(name))
+ if commit is None and tag is None and branch is None \
+ and refspec is None and url is not None:
+ raise RepoRefError('No commit, tag or branch specified for '
+ 'repository "{}". This is only allowed for '
+ 'local repositories.'.format(name))
if refspec is None:
commit = repo_overrides.get('commit', commit)
else:
if name not in Repo.__legacy_refspec_warned__:
- logging.warning('Using deprecated refspec for repository '
- '"%s". You should migrate to commit/branch.',
+ logging.warning('Using deprecated refspec for repository "%s".'
+ ' You should migrate to commit/tag/branch.',
name)
Repo.__legacy_refspec_warned__.append(name)
- if commit is not None or branch is not None:
+ if commit is not None or tag is not None or branch is not None:
raise RepoRefError('Unsupported mixture of legacy refspec '
- 'and commit/branch for repository "{}"'
+ 'and commit/tag/branch for repository "{}"'
.format(name))
refspec = repo_overrides.get('commit', refspec)
path = repo_config.get('path', None)
@@ -209,10 +215,10 @@ class Repo:
disable_operations = True

if typ == 'git':
- return GitRepo(name, url, path, commit, branch, refspec, layers,
- patches, disable_operations)
+ return GitRepo(name, url, path, commit, tag, branch, refspec,
+ layers, patches, disable_operations)
if typ == 'hg':
- return MercurialRepo(name, url, path, commit, branch, refspec,
+ return MercurialRepo(name, url, path, commit, tag, branch, refspec,
layers, patches, disable_operations)
raise UnsupportedRepoTypeError('Repo type "%s" not supported.' % typ)

@@ -289,12 +295,12 @@ class RepoImpl(Repo):
'the remote url.')

# take what came out of clone and stick to that forever
- if self.commit is None and self.branch is None \
+ if self.commit is None and self.tag is None and self.branch is None \
and self.refspec is None:
return 0

if not get_context().update:
- # Do commit/branch/refspec exist in the current repository?
+ # Do commit/tag/branch/refspec exist in the current repository?
(retc, output) = await run_cmd_async(self.contains_refspec_cmd(),
cwd=self.path,
fail=False,
@@ -302,11 +308,12 @@ class RepoImpl(Repo):
if retc == 0:
logging.info('Repository %s already contains %s as %s',
self.name,
- self.commit or self.branch or self.refspec,
+ self.commit or self.tag or self.branch
+ or self.refspec,
output.strip())
return retc

- # Try to fetch if commit/branch/refspec is missing or if --update
+ # Try to fetch if commit/tag/branch/refspec is missing or if --update
# argument was passed
(retc, output) = await run_cmd_async(self.fetch_cmd(),
cwd=self.path,
@@ -323,8 +330,8 @@ class RepoImpl(Repo):
Checks out the correct revision of the repo.
"""
if self.operations_disabled \
- or (self.commit is None and self.branch is None
- and self.refspec is None):
+ or (self.commit is None and self.tag is None
+ and self.branch is None and self.refspec is None):
return

if not get_context().force_checkout:
@@ -339,6 +346,9 @@ class RepoImpl(Repo):
if self.commit:
desired_ref = self.commit
is_branch = False
+ elif self.tag:
+ desired_ref = self.tag
+ is_branch = False
else:
(_, output) = run_cmd(self.resolve_branch_cmd(),
cwd=self.path, fail=False)
@@ -438,9 +448,9 @@ class GitRepo(RepoImpl):
Provides the git functionality for a Repo.
"""

- def remove_ref_prefix(self, branch):
+ def remove_ref_prefix(self, ref):
ref_prefix = 'refs/'
- return branch[branch.startswith(ref_prefix) and len(ref_prefix):]
+ return ref[ref.startswith(ref_prefix) and len(ref_prefix):]

def add_cmd(self):
return ['git', 'add', '-A']
@@ -463,10 +473,13 @@ class GitRepo(RepoImpl):
branch = self.branch or self.refspec
if branch and branch.startswith('refs/'):
branch = 'remotes/origin/' + self.remove_ref_prefix(branch)
- return ['git', 'cat-file', '-t', self.commit or branch]
+ return ['git', 'cat-file', '-t', self.commit or self.tag or branch]

def fetch_cmd(self):
cmd = ['git', 'fetch', '-q']
+ if self.tag:
+ cmd.append('--tags')
+
branch = self.branch or self.refspec
if branch and branch.startswith('refs/'):
cmd.extend(['origin',
@@ -496,10 +509,10 @@ class GitRepo(RepoImpl):
return cmd

def prepare_patches_cmd(self):
- branch = self.branch or self.refspec
+ ref = self.tag or self.branch or self.refspec
return ['git', 'checkout', '-q', '-B',
'patched-{refspec}'.
- format(refspec=self.commit or self.remove_ref_prefix(branch))]
+ format(refspec=self.commit or self.remove_ref_prefix(ref))]

def apply_patches_file_cmd(self, path):
return ['git', 'apply', '--whitespace=nowarn', path]
@@ -526,7 +539,8 @@ class MercurialRepo(RepoImpl):
return ['hg', 'commit', '--user', 'kas <k...@example.com>', '-m', 'msg']

def contains_refspec_cmd(self):
- return ['hg', 'log', '-r', self.commit or self.branch or self.refspec]
+ return ['hg', 'log', '-r', self.commit or self.tag or self.branch
+ or self.refspec]

def fetch_cmd(self):
return ['hg', 'pull']
@@ -545,7 +559,7 @@ class MercurialRepo(RepoImpl):
return cmd

def prepare_patches_cmd(self):
- refspec = self.commit or self.branch or self.refspec
+ refspec = self.commit or self.tag or self.branch or self.refspec
return ['hg', 'branch', '-f',
'patched-{refspec}'.format(refspec=refspec)]

diff --git a/kas/schema-kas.json b/kas/schema-kas.json
index 65a3198..8b9fc8a 100644
--- a/kas/schema-kas.json
+++ b/kas/schema-kas.json
@@ -168,6 +168,9 @@
"branch": {
"type": "string"
},
+ "tag": {
+ "type": "string"
+ },
"refspec": {
"type": "string"
},
--
2.41.0


From e2d8e148ca2f12fb2ecd061126bb07f0acea0e4f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?F=C3=A9lix=20Pi=C3=A9dallu?= <fe...@piedallu.me>
Date: Fri, 28 Jul 2023 09:58:59 +0200
Subject: [PATCH 2/2] Add tests for tag property of repo
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Signed-off-by: Félix Piédallu <fe...@piedallu.me>
---
tests/test_refspec.py | 18 ++++++++++++++++++
tests/test_refspec/test.yml | 8 ++++++++
tests/test_refspec/test2.yml | 8 ++++++++
tests/test_refspec/test3.yml | 4 ++++
tests/test_refspec/test7.yml | 10 ++++++++++
5 files changed, 48 insertions(+)
create mode 100644 tests/test_refspec/test7.yml

diff --git a/tests/test_refspec.py b/tests/test_refspec.py
index c1dfcde..cfc89a7 100644
--- a/tests/test_refspec.py
+++ b/tests/test_refspec.py
@@ -50,6 +50,10 @@ def test_refspec_switch(changedir, tmpdir):
fail=False, liveupdate=False)
assert rc == 0
assert output.strip() == 'refs/heads/master'
+ (rc, output) = run_cmd(['git', 'tag', '--points-at', 'HEAD'], cwd='kas3',
+ fail=False, liveupdate=False)
+ assert rc == 0
+ assert output.strip() == '3.0.1'

kas.kas(['shell', 'test2.yml', '-c', 'true'])
(rc, output) = run_cmd(['git', 'symbolic-ref', '-q', 'HEAD'], cwd='kas',
@@ -64,6 +68,14 @@ def test_refspec_switch(changedir, tmpdir):
fail=False, liveupdate=False)
assert rc == 0
assert output.strip() == '907816a5c4094b59a36aec12226e71c461c05b77'
+ (rc, output) = run_cmd(['git', 'symbolic-ref', '-q', 'HEAD'], cwd='kas3',
+ fail=False, liveupdate=False)
+ assert rc == 0
+ assert output.strip() == 'refs/heads/master'
+ (rc, output) = run_cmd(['git', 'tag', '--points-at', 'HEAD'], cwd='kas4',
+ fail=False, liveupdate=False)
+ assert rc == 0
+ assert output.strip() == '2.6.3'


def test_refspec_absolute(changedir, tmpdir):
@@ -87,6 +99,10 @@ def test_refspec_absolute(changedir, tmpdir):
cwd='kas_rel', fail=False, liveupdate=False)
assert rc == 0
assert output_kas_abs.strip() == output_kas_rel.strip()
+ (rc, output) = run_cmd(['git', 'tag', '--points-at', 'HEAD'],
+ cwd='kas_tag_abs', fail=False, liveupdate=False)
+ assert rc == 0
+ assert output.strip() == '3.0.1'


def test_url_no_refspec(changedir, tmpdir):
@@ -111,6 +127,8 @@ def test_commit_refspec_mix(changedir, tmpdir):
kas.kas(['shell', 'test5.yml', '-c', 'true'])
with pytest.raises(RepoRefError):
kas.kas(['shell', 'test6.yml', '-c', 'true'])
+ with pytest.raises(RepoRefError):
+ kas.kas(['shell', 'test7.yml', '-c', 'true'])


def test_refspec_warning(capsys, changedir, tmpdir):
diff --git a/tests/test_refspec/test.yml b/tests/test_refspec/test.yml
index c15d27d..d00927c 100644
--- a/tests/test_refspec/test.yml
+++ b/tests/test_refspec/test.yml
@@ -11,3 +11,11 @@ repos:
kas2:
url: https://github.com/siemens/kas.git
branch: master
+
+ kas3:
+ url: https://github.com/siemens/kas.git
+ tag: 3.0.1
+
+ kas4:
+ url: https://github.com/siemens/kas.git
+ tag: master
diff --git a/tests/test_refspec/test2.yml b/tests/test_refspec/test2.yml
index ae6fb4e..e68cf52 100644
--- a/tests/test_refspec/test2.yml
+++ b/tests/test_refspec/test2.yml
@@ -12,3 +12,11 @@ repos:
url: https://github.com/siemens/kas.git
# keep legacy refspec here for testing purposes
refspec: 907816a5c4094b59a36aec12226e71c461c05b77
+
+ kas3:
+ url: https://github.com/siemens/kas.git
+ branch: master
+
+ kas4:
+ url: https://github.com/siemens/kas.git
+ tag: 2.6.3
diff --git a/tests/test_refspec/test3.yml b/tests/test_refspec/test3.yml
index 41e3859..7f8b4ce 100644
--- a/tests/test_refspec/test3.yml
+++ b/tests/test_refspec/test3.yml
@@ -11,3 +11,7 @@ repos:
kas_rel:
url: https://github.com/siemens/kas.git
branch: master
+
+ kas_tag_abs:
+ url: https://github.com/siemens/kas.git
+ tag: refs/tags/3.0.1
diff --git a/tests/test_refspec/test7.yml b/tests/test_refspec/test7.yml
new file mode 100644
index 0000000..76447c9
--- /dev/null
+++ b/tests/test_refspec/test7.yml
@@ -0,0 +1,10 @@
+header:
+ version: 14
+
+repos:
+ this:
+
+ kas:
+ url: https://github.com/siemens/kas.git
+ refspec: dc44638cd87c4d0045ea2ca441e682f3525d8b91
+ tag: 3.0.1
--
2.41.0

[Jan Kiszka]

On 08.08.23 10:54, fe...@piedallu.me wrote:
> Hi Jan,
>
>> Unfortunately, it's hard to comment on your patches as you didn't
>> send them inline (git send-email may help)
>
> Ah, sorry. My org blocks SMTP so git send-email wasn’t usable.
> I pasted my patches in this email.
>

No guest wifi or VPN tunnels or other bypasses around? ;)

>> Now, a tag does not fall into any of these scenarios. It's normally
>> not floating but it is also not stable (only a commit is). Why do we
>> still want it then?
>
> The reason we want tags instead of SHAs is that SHAs are not readable.
> You can’t tell easily what "release" it’s pointing. That’s what tags are for :)
>
> I fully agree tags are not stable, but they should be expected to be so,
> *as long as you trust the repository* of course. That is the case for
> organisation internal repositories, where tags are generaly used.
>
> Also, I’m working with multi-repo orgs that create the same tag on all the
> repositories ; effectively allowing devs to ensure all repos are in sync in
> their local copy or Kas config files.
>
>
> That said, I have an idea about how we could make tags more trustworthy:
>
> * If the repo only declares `tag`: this isn’t trustworthy and might trigger a
> runtime warning;
> * If the repo declares `tag` and `commit`: Kas should check that the given
> `tag` resolves to the given `commit` and, if not, should *error* out.
>
> This strategy could ensure trustworthy *and* readable repo declarations.
>
> I believe the current behaviour implemented by my patches is to fail if both
> `commit` and `tag` are given, just like `commit` and `branch`.
>
>
> Would you consider this ?

This sounds like a reasonable path, yes. Could be combined with lock
files so that you don't need to figure out the commit SHAs manually.

Please then also add the corresponding test cases for these cases.

[Félix Piédallu (Salamandar)]

Thank you for your reply.

> No guest wifi or VPN tunnels or other bypasses around? ;)

Unfortunately no, even when working from home...

> Please then also add the corresponding test cases for these cases.

Working on this !

Félix Piédallu

[Félix Piédallu]

This second version of patches adds the possibility to provide both
'tag' and 'commit', and kas will check that they match and error out
if not.
Considering that tags are not safe (because not immutable), kas will
issue a warning if 'tag' is provided without 'commit'.

[Félix Piédallu (Salamandar)]

It looks like my patches take some time to arrive.

On a side note, I forgot to update the userguide. If the rest of the patch is OK for you i’ll update it.

Félix Piédallu

[Félix Piédallu]

Tags usually never move, but they aren't immutable, so a configuration
can reference commit and tag at the same time.
Kas will check that the commit and tag match, and if not, error out.

If no commit is provided with the tag, kas will issue a warning about
the unsafeness of tags.

Signed-off-by: Félix Piédallu <fe...@piedallu.me>
---
docs/userguide.rst | 4 ++
kas/libkas.py | 4 +-

kas/plugins/dump.py | 8 +--
kas/plugins/for_all_repos.py | 5 ++
kas/repos.py | 101 +++++++++++++++++++++++++----------
kas/schema-kas.json | 3 ++
6 files changed, 90 insertions(+), 35 deletions(-)

index 95038c1..a73ff24 100644

--- a/kas/repos.py
+++ b/kas/repos.py
@@ -76,12 +76,13 @@ class Repo:
Represents a repository in the kas configuration.
"""

- def __init__(self, name, url, path, commit, branch, refspec, layers,
+ def __init__(self, name, url, path, commit, tag, branch, refspec, layers,
patches, disable_operations):
self.name = name
self.url = url
self.path = path
self.commit = commit
+ self.tag = tag
self.branch = branch
self.refspec = refspec
self._layers = layers
@@ -113,6 +114,8 @@ class Repo:
elif item == 'revision':
if self.commit:
return self.commit
+ if self.tag:
+ return self.tag
branch = self.branch or self.refspec
if not branch:
return None

@@ -126,14 +129,17 @@ class Repo:

raise AttributeError

def __str__(self):
- if self.commit and self.branch:
- return '%s:%s(%s) %s %s' % (self.url, self.commit, self.branch,
+ if self.commit and (self.tag or self.branch):
+ return '%s:%s(%s) %s %s' % (self.url, self.commit,
+ self.tag or self.branch,
self.path, self._layers)
return '%s:%s %s %s' % (self.url,
- self.commit or self.branch or self.refspec,
+ self.commit or self.tag or self.branch
+ or self.refspec,
self.path, self._layers)

__legacy_refspec_warned__ = []

+ __no_commit_tag_warned__ = []

@staticmethod
def factory(name, repo_config, repo_defaults, repo_fallback_path,
@@ -168,27 +174,34 @@ class Repo:

+ if tag and not commit:
+ if name not in Repo.__no_commit_tag_warned__:
+ logging.warning('Using tag without commit for repository '
+ '"%s". This is unsafe as tags are not '
+ 'immutable, thus it is discouraged.', name)
+ Repo.__no_commit_tag_warned__.append(name)

path = repo_config.get('path', None)

disable_operations = False

@@ -209,10 +222,10 @@ class Repo:

disable_operations = True

if typ == 'git':
- return GitRepo(name, url, path, commit, branch, refspec, layers,
- patches, disable_operations)
+ return GitRepo(name, url, path, commit, tag, branch, refspec,
+ layers, patches, disable_operations)
if typ == 'hg':
- return MercurialRepo(name, url, path, commit, branch, refspec,
+ return MercurialRepo(name, url, path, commit, tag, branch, refspec,
layers, patches, disable_operations)
raise UnsupportedRepoTypeError('Repo type "%s" not supported.' % typ)

@@ -289,12 +302,12 @@ class RepoImpl(Repo):

'the remote url.')

# take what came out of clone and stick to that forever
- if self.commit is None and self.branch is None \
+ if self.commit is None and self.tag is None and self.branch is None \
and self.refspec is None:
return 0

if not get_context().update:
- # Do commit/branch/refspec exist in the current repository?
+ # Do commit/tag/branch/refspec exist in the current repository?
(retc, output) = await run_cmd_async(self.contains_refspec_cmd(),
cwd=self.path,
fail=False,

@@ -302,11 +315,12 @@ class RepoImpl(Repo):

if retc == 0:
logging.info('Repository %s already contains %s as %s',
self.name,
- self.commit or self.branch or self.refspec,
+ self.commit or self.tag or self.branch
+ or self.refspec,
output.strip())
return retc

- # Try to fetch if commit/branch/refspec is missing or if --update
+ # Try to fetch if commit/tag/branch/refspec is missing or if --update
# argument was passed
(retc, output) = await run_cmd_async(self.fetch_cmd(),
cwd=self.path,

@@ -323,8 +337,8 @@ class RepoImpl(Repo):

Checks out the correct revision of the repo.
"""
if self.operations_disabled \
- or (self.commit is None and self.branch is None
- and self.refspec is None):
+ or (self.commit is None and self.tag is None
+ and self.branch is None and self.refspec is None):
return

if not get_context().force_checkout:

@@ -336,9 +350,27 @@ class RepoImpl(Repo):
logging.warning('Repo %s is dirty - no checkout', self.name)
return

+ if self.tag:
+ (retc, output) = run_cmd(self.resolve_tag_cmd(),
+ cwd=self.path,
+ fail=False)
+ if retc:
+ raise RepoRefError(
+ 'Tag "{}" cannot be found in repository {}'
+ .format(self.tag, self.name))
+
+ if self.commit and output.strip() != self.commit:
+ # Ensure provided commit and tag match
+ raise RepoRefError('Provided tag "{}" does not match provided '
+ 'commit "{}" in repository "{}", aborting!'
+ .format(self.tag, self.commit, self.name))
+

if self.commit:
desired_ref = self.commit
is_branch = False
+ elif self.tag:
+ desired_ref = self.tag
+ is_branch = False
else:
(_, output) = run_cmd(self.resolve_branch_cmd(),
cwd=self.path, fail=False)

@@ -438,9 +470,9 @@ class GitRepo(RepoImpl):

Provides the git functionality for a Repo.
"""

- def remove_ref_prefix(self, branch):
+ def remove_ref_prefix(self, ref):
ref_prefix = 'refs/'
- return branch[branch.startswith(ref_prefix) and len(ref_prefix):]
+ return ref[ref.startswith(ref_prefix) and len(ref_prefix):]

def add_cmd(self):
return ['git', 'add', '-A']

@@ -463,10 +495,13 @@ class GitRepo(RepoImpl):

branch = self.branch or self.refspec
if branch and branch.startswith('refs/'):
branch = 'remotes/origin/' + self.remove_ref_prefix(branch)
- return ['git', 'cat-file', '-t', self.commit or branch]
+ return ['git', 'cat-file', '-t', self.commit or self.tag or branch]

def fetch_cmd(self):
cmd = ['git', 'fetch', '-q']
+ if self.tag:
+ cmd.append('--tags')
+
branch = self.branch or self.refspec
if branch and branch.startswith('refs/'):
cmd.extend(['origin',

@@ -485,6 +520,10 @@ class GitRepo(RepoImpl):
format(branch=self.remove_ref_prefix(
self.branch or self.refspec))]

+ def resolve_tag_cmd(self):
+ return ['git', 'rev-list', '-n', '1',
+ self.remove_ref_prefix(self.tag)]
+
def checkout_cmd(self, desired_ref, is_branch):
cmd = ['git', 'checkout', '-q', self.remove_ref_prefix(desired_ref)]
if is_branch:
@@ -496,10 +535,10 @@ class GitRepo(RepoImpl):

return cmd

def prepare_patches_cmd(self):
- branch = self.branch or self.refspec
+ ref = self.tag or self.branch or self.refspec
return ['git', 'checkout', '-q', '-B',
'patched-{refspec}'.
- format(refspec=self.commit or self.remove_ref_prefix(branch))]
+ format(refspec=self.commit or self.remove_ref_prefix(ref))]

def apply_patches_file_cmd(self, path):
return ['git', 'apply', '--whitespace=nowarn', path]

@@ -526,7 +565,8 @@ class MercurialRepo(RepoImpl):

return ['hg', 'commit', '--user', 'kas <k...@example.com>', '-m', 'msg']

def contains_refspec_cmd(self):
- return ['hg', 'log', '-r', self.commit or self.branch or self.refspec]
+ return ['hg', 'log', '-r', self.commit or self.tag or self.branch
+ or self.refspec]

def fetch_cmd(self):
return ['hg', 'pull']

@@ -538,6 +578,9 @@ class MercurialRepo(RepoImpl):
return ['hg', 'identify', '--id', '-r', self.branch or self.refspec,
'default']

+ def resolve_tag_cmd(self):
+ return self.resolve_branch_cmd()
+
def checkout_cmd(self, desired_ref, is_branch):
cmd = ['hg', 'checkout', desired_ref]
if get_context().force_checkout:
@@ -545,7 +588,7 @@ class MercurialRepo(RepoImpl):

return cmd

def prepare_patches_cmd(self):
- refspec = self.commit or self.branch or self.refspec
+ refspec = self.commit or self.tag or self.branch or self.refspec
return ['hg', 'branch', '-f',
'patched-{refspec}'.format(refspec=refspec)]

diff --git a/kas/schema-kas.json b/kas/schema-kas.json
index 65a3198..8b9fc8a 100644
--- a/kas/schema-kas.json
+++ b/kas/schema-kas.json
@@ -168,6 +168,9 @@
"branch": {
"type": "string"
},
+ "tag": {
+ "type": "string"
+ },
"refspec": {
"type": "string"
},
--

2.30.2

[Félix Piédallu]

* test_refspec_switch
* test mix of refspec and tag
* test if kas fails if commit and tag do not match
* test if kas warns about tag without commit being unsafe

Signed-off-by: Félix Piédallu <fe...@piedallu.me>
---

tests/test_refspec.py | 44 ++++++++++++++++++++++++++++++++++++
tests/test_refspec/test.yml | 9 ++++++++
tests/test_refspec/test2.yml | 8 +++++++

tests/test_refspec/test3.yml | 4 ++++
tests/test_refspec/test7.yml | 10 ++++++++

tests/test_refspec/test8.yml | 10 ++++++++
6 files changed, 85 insertions(+)
create mode 100644 tests/test_refspec/test7.yml
create mode 100644 tests/test_refspec/test8.yml

diff --git a/tests/test_refspec.py b/tests/test_refspec.py
index c1dfcde..b483fc2 100644

@@ -111,6 +127,34 @@ def test_commit_refspec_mix(changedir, tmpdir):

kas.kas(['shell', 'test5.yml', '-c', 'true'])
with pytest.raises(RepoRefError):
kas.kas(['shell', 'test6.yml', '-c', 'true'])
+ with pytest.raises(RepoRefError):
+ kas.kas(['shell', 'test7.yml', '-c', 'true'])

+
+
+def test_tag_commit_do_not_match(changedir, tmpdir):
+ """
+ Test that giving tag and commit that do not match raises an error.
+ """
+ tdir = str(tmpdir / 'test_tag_commit_do_not_match')
+ shutil.copytree('tests/test_refspec', tdir)
+ os.chdir(tdir)
+ with pytest.raises(RepoRefError):
+ kas.kas(['shell', 'test8.yml', '-c', 'true'])
+
+
+def test_unsafe_tag_warning(capsys, changedir, tmpdir):
+ """
+ Test that using tag without commit issues a warning, but only once.
+ """
+ tdir = str(tmpdir / 'test_unsafe_tag_warning')
+ shutil.copytree('tests/test_refspec', tdir)
+ os.chdir(tdir)
+ # needs to be reset in case other tests ran before
+ Repo.__no_commit_tag_warned__ = []
+ kas.kas(['shell', 'test2.yml', '-c', 'true'])
+ assert capsys.readouterr().err.count(
+ 'Using tag without commit for repository "kas4". This is unsafe as '
+ 'tags are not immutable, thus it is discouraged.') == 1

def test_refspec_warning(capsys, changedir, tmpdir):
diff --git a/tests/test_refspec/test.yml b/tests/test_refspec/test.yml

index c15d27d..4b17491 100644
--- a/tests/test_refspec/test.yml
+++ b/tests/test_refspec/test.yml
@@ -11,3 +11,12 @@ repos:

kas2:
url: https://github.com/siemens/kas.git
branch: master
+
+ kas3:
+ url: https://github.com/siemens/kas.git
+ tag: 3.0.1

+ commit: 229310958b17dc2b505b789c1cc1d0e2fddccc44

diff --git a/tests/test_refspec/test8.yml b/tests/test_refspec/test8.yml
new file mode 100644
index 0000000..eb312cd
--- /dev/null
+++ b/tests/test_refspec/test8.yml

@@ -0,0 +1,10 @@
+header:
+ version: 14
+
+repos:
+ this:
+
+ kas:
+ url: https://github.com/siemens/kas.git

+ tag: 3.0.1
+ commit: de4dcafe
--
2.30.2

[Jan Kiszka]

And if both commit and tag are provided, it is checked if they match, right?

What is the behavior for the other corner cases, namely tag+branch but
no commit and tag+branch+commit?

Looks generally good, but we need to double-check that the corner cases
have well-defined behaviour (and then probably also test cases).
Furthermore, others should have a look at this code as well.

[Félix Piédallu (Salamandar)]

Hi,

> And if both commit and tag are provided, it is checked if they match, right?

Yeah, i forgot to update the userguide in my last patch.

> What is the behavior for the other corner cases, namely 

> tag+branch but no commit

branch is ignored, tag is used, and a warning is issues about commit not present.

I’m not sure this is a good use case. Would we want an error about tag and branch conflicting ?

Or would we want to check that `git branch --contains ${tag}` ?

> tag+branch+commit?

Same as above branch is ignored, and tag is checked against commit.

> we need to double-check that the corner cases have well-defined behaviour

Do you think about other corner cases right now ?

Félix Piédallu

[MOESSBAUER, Felix]

On Thu, 2023-08-17 at 18:31 +0200, Félix Piédallu wrote:
> Tags usually never move, but they aren't immutable, so a
> configuration
> can reference commit and tag at the same time.
> Kas will check that the commit and tag match, and if not, error out.
>
> If no commit is provided with the tag, kas will issue a warning about
> the unsafeness of tags.

Hi Felix,

will this warning eventually become an error (like the refspec
property)? If not, I would just remove this warning, as we don't have
one with the branch as well.

Copy paste error: if no **tag** was given.

I recommend to make it a bit shorter (personal opinion only):
"Using tag without commit for repository <repo> is unsafe as tags are
mutable."

I'm not a HG expert, but does this actually work with tags? Has this
been tested?

>  
>      def fetch_cmd(self):
>          return ['hg', 'pull']
> @@ -538,6 +578,9 @@ class MercurialRepo(RepoImpl):
>          return ['hg', 'identify', '--id', '-r', self.branch or
> self.refspec,
>                  'default']
>  
> +    def resolve_tag_cmd(self):
> +        return self.resolve_branch_cmd()
> +

Tags and branches are not the same. Please briefly explain why this
works and also resolves correctly in case identical branch and tag
names exist with different SHAs (which is discouraged but supported in
git).

In general, this already looks pretty good.
When preparing the v3, please also rebase against next and send
directly to the ML (no --in-reply-to).

Felix

[MOESSBAUER, Felix]

On Thu, 2023-08-17 at 18:31 +0200, Félix Piédallu wrote:

> * test_refspec_switch
> * test mix of refspec and tag
> * test if kas fails if commit and tag do not match
> * test if kas warns about tag without commit being unsafe

Hi,

please also add a test where a branch and tag with identical name but
different SHA exists.

Example:

git checkout master
git tag foo
git checkout HEAD~
git checkout -b foo

Best regards,
Felix

[Jan Kiszka]

On 02.09.23 11:00, 'MOESSBAUER, Felix' via kas-devel wrote:
> On Thu, 2023-08-17 at 18:31 +0200, Félix Piédallu wrote:
>> * test_refspec_switch
>> * test mix of refspec and tag
>> * test if kas fails if commit and tag do not match
>> * test if kas warns about tag without commit being unsafe
>
> Hi,
>
> please also add a test where a branch and tag with identical name but
> different SHA exists.
>
> Example:
>
> git checkout master
> git tag foo
> git checkout HEAD~
> git checkout -b foo
>

Were there any follow-ups after this that I missed? If this is supposed
to make it into the next release, now would be a good time to provide an
update.

[Félix Piédallu]

Hi,

I'm sorry, I didn't have much time on my hands to finish this. I can
send the final patches on thursday.

Félix

[Jan Kiszka]

On 08.10.23 18:55, Félix Piédallu wrote:
> Hi,
>
> I'm sorry, I didn't have much time on my hands to finish this. I can
> send the final patches on thursday.
>

No problem, Thursday would be great!

Jan

> Félix
>
> Le 07/10/2023 à 07:44, 'Jan Kiszka' via kas-devel a écrit :
>> On 02.09.23 11:00, 'MOESSBAUER, Felix' via kas-devel wrote:
>>> On Thu, 2023-08-17 at 18:31 +0200, Félix Piédallu wrote:
>>>> * test_refspec_switch
>>>> * test mix of refspec and tag
>>>> * test if kas fails if commit and tag do not match
>>>> * test if kas warns about tag without commit being unsafe
>>> Hi,
>>>
>>> please also add a test where a branch and tag with identical name but
>>> different SHA exists.
>>>
>>> Example:
>>>
>>> git checkout master
>>> git tag foo
>>> git checkout HEAD~
>>> git checkout -b foo
>>>
>> Were there any follow-ups after this that I missed? If this is supposed
>> to make it into the next release, now would be a good time to provide an
>> update.
>>
>> Jan
>>
>