Currently git-debpush has a bunch of checks, and options to --force
override those checks.

I would like it to have a new check that helps the average user avoid
tagging and uploading commits that did not pass the CI (typically
Salsa CI).

The logic could be something along the lines:

1. Read the git remote URL or d/control Vcs-Browse field to deduce
what is the Salsa project page for the package (if any) -
alternatively skip check if package isn't on Salsa

2. Fetch pipelines for the package with something that is equivalent to:

curl --silent "https://salsa.debian.org/go-team/packages/golang-github-ziutek-telnet/-/pipelines.json?scope=all&page=1"
| jq '.pipelines[] | [.commit.short_id, .details.status.label]'
[
  "b7e77496",
  "pending"
]
[
  "1abf88e1",
  "passed"
]
[
  "1abf88e1",
  "passed"
]

3. Check if the current git HEAD (or if --branch was passed the HEAD
there) commit ID is in the list of pipelines, and if it has state
'passed'. Alternatively skip rest of the check if the package does not
have pipelines and does not appear to be using CI.

4. Succeed and continue with tagging and pushing or fail with useful
error message depending on situation:
a) commit was not pushed yet - please push and wait until CI signals all green
b) commit was pushed but state 'pending' - please wait and try again
c) commit was pushed but state 'failed' - show a link to pipeline and
request user to investigate and make CI green before pushing, or
re-run git-debpush --force=ci-failure to override

I don't know, maybe we could even consider having a note field too so
people can do something like `git-debpush
--ignore-ci-reason="Autopkgtest cannot pass before this version is
uploaded due to circular depenency"` which could be recorded in the
git tag and then other people working on the same repository would
have a chance to understand why another developer intentionally
uploaded despite failing CI.

Another idea to throw out: Maybe in some cases git-debpush might even
just keep running if job state was 'pending' and poll the pipeline
once every 3 minutes or so until it passed and then automatically
proceed.

What do you think, would this make sense? Or should the CI check be at
some other level of the tag2upload pipeline? Having it in the
git-debpush itself and allowing developers to override it would
probably be the most flexible solution and give enough soft
enforcement that most people would stop and think before uploading
stuff that is known to be broken in CI.

Hello,

git-debpush is generally a pure git tool; its checks are all git-related
or about pure package metadata.  Checking that CI passed is sort of like
checking that the package successfully builds -- we don't require that,
or anything like it, at present.

It would also slow things down to be making additional network calls
like this.

So my instinct would be that this is out-of-scope.

Network calls are cheap, and it is already doing them to
salsa.debian.org. It is way more expensive to have humans -
potentially many of them - to debug what is going on when their work
got halted by interactions with a broken package in the Debian
archive. Doing cleanup type of work is also very demotivating for
volunteers, and utilizing CI helps by preventing problems and the need
for cleanup to arise in the first place. If Salsa CI exists for a
package I think we should make it as easy as possible for people to
use it.

If you don't want git-debpush to have this new check along the other
checks it has, what do you think would be a better place to do this CI
gatekeeping in the tag2upload pipeline? To me using git-debpush seems
quite ideal for this, especially as it already has a list of various
checks.

Hello,

They often aren't.  Not everyone has a great connection.  Maybe you
happy to live especially close to where salsa is hosted, or something :)

My instinct is that it is not a tag2upload issue at all.

It seems like a step between sbuild and git-debpush.

Hi,

If this feature were to be implemented as requested, I can imagine a
scenario where someone who has until now not noticed salsa-CI might
become aware of it only when git-debpush refuses to do what they want.

I'd guess that might well provoke a negative reaction, and the blame
would either fall on git-debpush or salsa-CI, whereas it should really
be pointed at whoever in their team enabled salsa-CI and then allowed it
to end up in a state where it routinely fails.

Might it be better to have an option for a pre-tag hook?

That would then allow one to add checks of such things as the state of
salsa-CI, in an opt-in manner where only people that know they want it
will ever be told that they need to wait for CI to complete.

Also, my workflow often involves repeatedly pushing slightly dodgy
versions at a repo in my namespace until I am eventually happy with the
outcome (including CI passing). Only then do I try to do an upload.

If I was doing that with git-debpush, I'd want to be able to have it
check the state of CI in the repo under _my_ namespace -- not in the
upstream repo I was finally going to push the release commit into.

I wouldn't want to have to wait for the CI to be run again, before the
upload completes, especially if that requires me to be online from start
to finish -- what it one goes offline before the extra CI run completes?

Cheers, Phil.

Philip Hands wrote:

Please note what I wrote in

So person running git-debpush would only see message about Salsa CI
not passing if the project is using Salsa CI. So if somebody "until
now not noticed Salsa CI" because their project is not using it would
not see anything. If the project is using it, then seeing the pipeline
status is very much intentional and the whole point of the feature.

So you would like all developers to manually install a pre-tag hook
for themselves instead of having the git tag tool do this
automatically? I think the people diligently installing pre-tag-hooks
are likely the same who already know if their project is building and
passing CI. The benefit of having this feature in git-debpush is to
raise the CI status to people who might not otherwise make effort to
check it, thus providing a nice convenience service to developers.

Philip Hands writes ("Bug#1111331: pre-tag hook as an alternative?"):
this interesting.

The idea of a pre-push hook seems a reasonable one - even though it
probably wouldn't meet everyone's use case for this ticket, it might
be useful for other things too.

These ideas are all at an early stage and this kind of input is very
helpful.  It will help us know what shape of thing(s) we want to try
to make.  We might well do more than one thing.

Otto Kekäläinen writes ("Bug#1111331: git-debpush: check if CI passed before tagging"):

On the other hand, I found this response discouraging and
dogmatic-sounding.

It's true that some ways of implementing Phil's suggestion might have
some downsides, but this whole area is full of tradeoffs.  Many of the
observations you make are fairly obvious and didn't need to be stated.
Non-obvious downsides can be stated in a more neutral way.

Otto, please reconsider your comunication style. [1]

Ian.

[1] Note that this is a formal moderation intervention from one of the
src:dgit maintainers.  We are responsible for curating the community
around our software, and take that responsibility seriously.

This message is not an invitation to a meta-debate.  If you don't
understand what I've said above, or disagree with it, please mail
dgit@packages.d.o.  If necessary Sean can provide a second opinion.
Your best escalation route beyond that is to the Community Team.

I've been thinking about this and I'm inclined to think the best way
to support the "fire and forget" user story with Salsa is:

 * The tag contains an instruction that CI should have passed,
   and the identity of the forge.  (Its domain name, I guess.)

 * The Manager uses Salsa's API and webhook system to determine
   when CI has passed.  The Manager passes the job to the Oracle
   only when that is the case.  The Oracle trusts the Manager on this
   point.

 * If CI fails, Salsa will hopefully email someone.  But the Manager
   should also email the tagger.  There should be at most one such
   mail.  CI failure is not fatal for the upload, though.  If the CI
   should pass later (eg someone hit "retry"), the upload will go
   ahead.

 * If CI fails for cause and code update is needed, the user must burn
   the version number and make a new changelog stanza.

What I don't know is how git-debpush should know that it should put
this CI pass instruction in the tag.  One option is to say that it
does that by default when the remote is salsa.d.o - so the default is
to require CI to pass if there is a pipeline.

Ian.

Hello,

These ideas are very interesting.  The last question, about git-debpush,
does indeed seem unresolved here.

For two complex packages I upload, sbcl and Emacs, the CI pipelines on
salsa were set up by other people and always fail. I've never looked at
them, leaving it to them.  So such a default would very much get in my
way.  So ideally we can come up with something more subtle.

Sean Whitton writes ("Bug#1111331: git-debpush: check if CI passed before tagging"):

I'm tempted to suggest that if you have a CI pipeline that always
fails you ought to get rid of it :-).

But more practically, would it be terrible if this situation required
configuration somewhere?  Places this could go include
 (a) git config in your local tree
 (b) something in the git tree object
 (c) command line option
I think (b) is probably an OK choice.

At some point we're going to want to do maintainer-workflow-agnostic
NMUs.  This is yet another case where the non-maintainer-workflow
upload needs different handling.  I guess a non-maintainer-workflow
NMU is going to have a different remote since it mustn't push the dgit
view tag with the DEP0-14 name to salsa.  So that's a way to tell them
apart.

So how about a t2u config file in debian/, somewhere?  For your
packages with broken CI it oculd say "disregard CI for
salsa.debian.org" or "disregard CI for all gitlab forges".

Ian.

Hello,

Yeah.  In these cases it's not really mine to delete, basically.

Yes, (b) would be okay.

So far we are reusing debian/source/options for 1.0 and are planning to
use some of debian/gbp.conf.  But having our own file would be okay.
(And dpkg is newly complaining about our new of d/s/options, so, we
might want to move that.)

How about debian/git-debpush.ini ?

Sean Whitton writes ("Bug#1111331: git-debpush: check if CI passed before tagging"):

I do think we want our own file.  (The things we put in d/s/options
ought to be there still I think.)

Can I galk you into TOML ?

Ian.

Hello,

Thinking about it a little more systematically ISTM that some good
criteria might be:

- similar to other stuff in debian/

  I think that means deb822 (control), ini (gbp.conf) or unsectioned
  line-based (debhelper configs).

- easy to parse enough for how git-debpush is a shell script.

I'm not sure TOML fits these criteria.
But maybe you could share why you like it?

I think this anecdote is actually quite relevant to this design discussion:

If you look at https://salsa.debian.org/common-lisp-team/sbcl/-/pipelines?page=5&scope=all
you can see the CI was passing in late 2022 and started failing on
Lintian errors in your commit in 2023. If I was the maintainer, I
would stop all development and fix the failure. If it is not fixed
immediately, people will get "alert fatigue" and the CI will continue
to regress more and it is unlikely nobody will ever fix it. Now it is
so broken I'd recommend to just disable it completely, and not
re-enable it until someone has time to fix all the issues preventing
CI from passing.

Maintaining the package in a regression-free state (and compliant with
new Lintian versions and so forth) should not be the burden of one
diligent maintainer who goes around and cleans up after others made
changes and broke stuff. If a package has CI enabled, it should mean
that anyone touching the package needs to participate in maintaining
it. If maintaining CI is too hard due to e.g. large or complex package
issues, then the maintainers should agree to just decrease the CI
scope or completely disable it. If CI passes or not should be an
attribute for the package/project, not a per-maintainer preference
thing.

Thus my suggestion here is that all tag2uploads should check if CI is
enabled or not, and require that CI must be green. I don't think it is
a good idea to introduce a new config file in the context of CI (but
maybe you have other things that need configuration). The repository
commit/tag already has metadata in the forge if CI was enabled, and
that information should be used instead of requiring maintainers to
create new files to express the same thing.