packages in pipe report #680

glehmann · 2025-02-28T16:08:53Z

stormi · 2025-03-03T11:35:56Z

Looks like a good candidate for a sync review.

ydirson

There seems to be some overlap with #677 here. Also, do we want to depend on koji, when we can get the info from the repos directly?

stormi · 2025-03-03T16:02:41Z

There seems to be some overlap with #677 here. Also, do we want to depend on koji, when we can get the info from the repos directly?

You can't get the koji build ID and links to the web interface from repos directly.

ydirson

Also note the "hmtl" typo in commit message

scripts/pkg_in_pipe.py

ydirson · 2025-03-04T16:06:03Z

scripts/pkg_in_pipe.py

+                koji_output = check_output(['koji', 'buildinfo', build[0]]).decode('utf8')
+                kv_lines = [line.split(': ') for line in koji_output.splitlines()]
+                build_info = dict(kv for kv in kv_lines if len(kv) == 2)
+                print(build_info)


looks like debugging output, do we want to keep it?

ydirson · 2025-03-04T16:09:40Z

scripts/pkg_in_pipe.py

+    tags = [f'v{v}-{p}' for v in ['8.2', '8.3'] for p in ['incoming', 'ci', 'testing', 'candidates', 'lab']]
+    for tag in tags:


It does not seem necessary to precompute a list of tags. This and the lack of specific details about what exactly gets parallelized makes reading the patch harder than necessary

scripts/pkg_in_pipe.py

scripts/pkg_in_pipe/README.md

nathanael-h · 2025-03-07T16:54:56Z

scripts/pkg_in_pipe/README.md

+# Run in docker
+
+```sh
+docker build -f pkg_in_pipe .


Suggested change

docker build -f pkg_in_pipe .

docker buildx build . -t pkg_in_pipe

docker buildx replaces docker build see the "Important" note here https://docs.docker.com/reference/cli/docker/build-legacy/

docker build is an alias to docker buildx build and buildkit is the default build engine since docker 23, so I think we should be good with docker build :-)

unless we need to run an older version somewhere?

ref:

https://docs.docker.com/reference/cli/docker/buildx/build/

https://docs.docker.com/build/buildkit/

good catch for the -t!

nathanael-h · 2025-03-07T16:57:12Z

scripts/pkg_in_pipe/README.md

+
+```sh
+docker build -f pkg_in_pipe .
+docker run -v ~/.koji:/root/.koji:z -e PLANE_TOKEN=<plane token> -v /out/dir:/output:z pkg_in_pipe /output/index.html


We should document in README that a directory from the host should me mounted in the container, and that a positional argument is used to specify the output path and filename inside the container, shouldn't we?

nathanael-h · 2025-03-07T16:57:59Z

scripts/pkg_in_pipe/.gitignore

@@ -0,0 +1 @@
+report.html


Not needed anymore as the output is flexible, and the example in README.md is outside of the repo.

while not strictly needed, I think it's still nice to have it for local development

Yes, it is still the default output file.

glehmann · 2025-03-07T18:21:54Z

@nathanael-h I have also added the last generation time and generation interval in the report

ydirson · 2025-03-10T08:32:51Z

scripts/pkg_in_pipe/pkg_in_pipe.py

+def print_footer(out):
+    now = datetime.now()
+    print(f'''
+Last generated at {now}. Generated every 5 minutes.


While the "last generated" is useful info, hardcoding the interval of an external cron job does not feel right.

do you think it should be a command line option?

Could be a --generated-info="Generated every 5 minutes."? --cron-info="5 minutes"?

@nathanael-h as a consequence of this change, we'll have to add --generated-info "Generated every 5 minutes." on the command line when calling pkg_in_pipe.py

When the delay is short and people see that it was last generated less than 5 minutes ago, I think passing a parameter to tell them "it's generated every 5 minutes" is a bit overkill, but now that it's done, why not :)

scripts/pkg_in_pipe/README.md

stormi · 2025-04-08T19:59:28Z

@glehmann let's discuss it together with @nathanael-h in the coming days?

scripts/pkg_in_pipe/Dockerfile

scripts/pkg_in_pipe/README.md

ydirson · 2025-04-29T15:10:54Z

scripts/pkg_in_pipe/pkg_in_pipe.py

@@ -3,82 +3,91 @@
 import argparse
 import os
 from datetime import datetime
+from textwrap import dedent


this "style fixes" should likely be squashed into the first commit, no?

scripts/pkg_in_pipe/pkg_in_pipe.py

ydirson · 2025-04-29T16:11:43Z

scripts/pkg_in_pipe/pkg_in_pipe.py

+    except Exception as e:
+        ok = False
+        print(f"error while building the report: {e}", file=sys.stderr)
+        print_koji_error(out)


The print_koji_error error message is very specific, but gets triggered by a very wide except Exception. No need to wake up the koji admins if the problem is eg. a disk full while writing the report ;)

ydirson · 2025-04-29T16:13:25Z

scripts/pkg_in_pipe/pkg_in_pipe.py

+if not ok:
+    exit(1)


Wouldn't exit(1) or return 1 in the exception block do the job, with print_footer in a finally clause?

yes, not sure why I didn't do that 👍

ydirson · 2025-04-29T16:20:07Z

scripts/pkg_in_pipe/pkg_in_pipe.py

+# from icecream import ic
+


looks like remnants from a debugging session ;)

ydirson · 2025-04-29T16:33:29Z

scripts/pkg_in_pipe/pkg_in_pipe.py

+    tag = tag.split('-')[-1]
+    return TAG_ORDER.index(tag)
+
+def find_previous_build_commit(session, build_tag, build):


a short comment explaining it is not just the "previous build commit" could be useful

ydirson · 2025-04-29T16:35:36Z

scripts/pkg_in_pipe/pkg_in_pipe.py

+    for commit in gh.get_repo(repo).get_commits(start_sha):
+        if commit.sha == end_sha:


That idiom feels strange, as git history is not just linear. Isn't it possible do directly get a commit range from gh?
A docstring would help too

I haven't found a way to do that

https://docs.github.com/en/rest/commits/commits?apiVersion=2022-11-28#list-commits seems very limited indeed, apparently we would need a local clone to get the proper info (a "monorepo" would help here too). Maybe add a note then, for the time when we see strange results.

I'm not sure that's a problem: github linearizes the history in its commit pages.
We could check in a repo with a complex branch structure (like host-installer) if the commits made before the last commit in the base branch and merged in the current branch appear after the last commit

e15e appears after 6e8c in https://github.com/xcp-ng/host-installer/commits/ydi/10.10.28-8.3, but the merge commit, which is also considered to be in the PR appears before, so we're good for this use case.
I'll add a comment to warn about that.

GH linearizes in much the same way git log does, ie I guess it guaranties that all commits descending from the "limit commit" we're looking for will be see first. But other commits merged from a branch whose base does not include that "limit commit" may be listed after the "limit commit" (they bear no relation so order can be arbitrary), so they would be missed by this way of walking.

Question is, how much do we care?

Off the top of my head, I'd say this can happen every time we have a branch that gets merged quickly, while others have been open from some time (in which case the older commits are often sorted as older, and this the probability not to see then listed before the limit is really high). But then we're talking packaging repos only, and the management of package version already pushes towards rebasing before getting a PR merged - so it might not be that much of a problem, but could still occasionally happen.

I still think it warrants a note in the code explaining that this is the best we can reasonably do in the current state of GH APIs, and maybe point to this discussion for more details.

ydirson · 2025-04-29T16:42:04Z

scripts/pkg_in_pipe/pkg_in_pipe.py

    return sorted(prs, key=lambda p: p.number, reverse=True)

 parser = argparse.ArgumentParser(description='Generate a report of the packages in the pipe')
 parser.add_argument('output', nargs='?', help='Report output path', default='report.html')
 parser.add_argument('--generated-info', help="Add this message about the generation in the report")
-parser.add_argument('--plane-token', help="The token used to access the plane api", default=os.environ['PLANE_TOKEN'])


Removed by mistake, reintroduced with different indent in next commit (reinident would belong to first commit)

not a style fix: I used environ.get() to not raise an exception if the envar is not defined.
but indeed it was removed by mistake

ydirson · 2025-04-29T16:50:17Z

scripts/pkg_in_pipe/pkg_in_pipe.py

-gh = github.Github(auth=github.Auth.Token(os.environ['GITHUB_TOKEN']))
+if args.github_token:
+    gh = github.Github(auth=github.Auth.Token(args.github_token))
+    try:
+        gh.get_repo('xcp-ng/xcp')  # check that the token is valid
+    except BadCredentialsException:
+        gh = None
+else:
+    gh = None


"without github" does not seem completely accurate, since that covers "no token passed" and "bad credentials", but not any other error from GH, which happen from time to time.

Also, it looks like print_github_warning would claim "Github malfunction" when it's just "no token passed"?

That's inaccurate. I'll try something else.

ydirson · 2025-04-29T16:52:59Z

scripts/pkg_in_pipe/pkg_in_pipe.py

-                build_url = f'https://koji.xcp-ng.org/buildinfo?buildID={tagged['build_id']}'
+                    maintained_by = PACKAGES.get(repo.split('/')[-1], {}).get('maintainer')
+                build_url = f'https://koji.xcp-ng.org/buildinfo?buildID={tagged["build_id"]}'


It took me some concentration to see why that line is shown as modified, just to realized it does not belong to this commit

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech> Co-authored-by: Nathanaël <7300309+nathanael-h@users.noreply.github.com>

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

stormi · 2025-04-30T12:44:11Z

Removing myself from the code reviewers for this one. I'll continue providing feedback on the end result, independently of the implementation.

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

stormi requested review from stormi and ydirson March 3, 2025 11:35

ydirson reviewed Mar 3, 2025

View reviewed changes

ydirson requested changes Mar 4, 2025

View reviewed changes

glehmann force-pushed the pkg-in-pipe branch 2 times, most recently from 55ae5cb to 6f29e52 Compare March 6, 2025 10:40

nathanael-h reviewed Mar 7, 2025

View reviewed changes

scripts/pkg_in_pipe/README.md Outdated Show resolved Hide resolved

glehmann force-pushed the pkg-in-pipe branch from a317dc8 to 48b1a63 Compare March 7, 2025 14:59

nathanael-h reviewed Mar 7, 2025

View reviewed changes

glehmann force-pushed the pkg-in-pipe branch 2 times, most recently from 336d713 to f14946b Compare March 7, 2025 18:20

ydirson reviewed Mar 10, 2025

View reviewed changes

scripts/pkg_in_pipe/README.md Outdated Show resolved Hide resolved

glehmann force-pushed the pkg-in-pipe branch 3 times, most recently from 4b42ed2 to 10b532b Compare March 10, 2025 10:02

glehmann requested a review from ydirson March 14, 2025 08:07

glehmann force-pushed the pkg-in-pipe branch from 10b532b to 973c92d Compare April 7, 2025 14:16

glehmann force-pushed the pkg-in-pipe branch 3 times, most recently from 56bbe01 to 139539f Compare April 15, 2025 11:54

glehmann force-pushed the pkg-in-pipe branch 2 times, most recently from 03a273a to 42496a3 Compare April 25, 2025 12:21

nathanael-h reviewed Apr 25, 2025

View reviewed changes

scripts/pkg_in_pipe/Dockerfile Outdated Show resolved Hide resolved

nathanael-h reviewed Apr 25, 2025

View reviewed changes

scripts/pkg_in_pipe/README.md Outdated Show resolved Hide resolved

glehmann force-pushed the pkg-in-pipe branch from 3395eb1 to c258a97 Compare April 29, 2025 09:10

ydirson requested changes Apr 29, 2025

View reviewed changes

glehmann added 4 commits April 30, 2025 08:05

package in pipe html report generator

622fa7e

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

run pkg_in_pipe in docker

74c05e8

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

don't fail when plane or koji are not responding

bf0346c

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

sort the packages by reversed build id

dc7daa3

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

glehmann force-pushed the pkg-in-pipe branch from 7885071 to ca95fc6 Compare April 30, 2025 12:39

glehmann requested review from ydirson and nathanael-h April 30, 2025 12:40

glehmann and others added 3 commits April 30, 2025 14:43

add the pull request matching the koji build

725ad1d

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech> Co-authored-by: Nathanaël <7300309+nathanael-h@users.noreply.github.com>

also search the plane cards with the PR urls

37f8c7f

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

include all the PRs merged after the previous build

7fbe070

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

glehmann force-pushed the pkg-in-pipe branch from ca95fc6 to ac4eaf9 Compare April 30, 2025 12:43

stormi removed their request for review April 30, 2025 12:43

glehmann added 3 commits April 30, 2025 14:44

cache github calls

ec8652f

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

generate the report even without github

0a5bd0c

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

add a maintainer column

31f4928

Signed-off-by: Gaëtan Lehmann <gaetan.lehmann@vates.tech>

glehmann force-pushed the pkg-in-pipe branch from ac4eaf9 to 31f4928 Compare April 30, 2025 12:44

		tags = [f'v{v}-{p}' for v in ['8.2', '8.3'] for p in ['incoming', 'ci', 'testing', 'candidates', 'lab']]
		for tag in tags:

	docker build -f pkg_in_pipe .
	docker buildx build . -t pkg_in_pipe

		for commit in gh.get_repo(repo).get_commits(start_sha):
		if commit.sha == end_sha:

		@@ -0,0 +1 @@
		report.html

packages in pipe report #680

Are you sure you want to change the base?

packages in pipe report #680

Conversation

glehmann commented Feb 28, 2025 • edited Loading

stormi commented Mar 3, 2025

ydirson left a comment

Choose a reason for hiding this comment

stormi commented Mar 3, 2025

ydirson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glehmann Mar 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glehmann commented Mar 7, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stormi commented Apr 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glehmann Apr 30, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stormi commented Apr 30, 2025

glehmann commented Feb 28, 2025 •

edited

Loading

glehmann Mar 7, 2025 •

edited

Loading

glehmann Apr 30, 2025 •

edited

Loading