#1 Add project
Merged by dcavalca. Opened by oidoming.
centos-sig-hyperscale/ oidoming/package-updates main  into  main

Download 1.patch

Hi, PR for issue #54
At the first time running the script will request for git.centos.org release tags to create an issue for pending updates, then will listen on mqtt git.cent.org notifications for creating any future issues.
The script will create an issue like this in the package-bugs repo, the issue will have a tag with the package version (to avoid possible duplicates on restart), also if there is an open issue with a package version tag (created by the script) and there is a new update, the script will leave a comment on the previous issue, close it and create a new one (e.g. https://pagure.io/test-oidoming/issue/8).

Here is a diagram that describes how the script works.
More description available in README

We'll also need to manage the CentOS Steam 9 tags (I guess with a separate instance of this?) so you may wanna make these configurable.

I'd hope/expect koji has an API of some kind to fetch these instead of having to hardcode them (@ngompa do you know?)

General flow and logic lgtm (and thank you for writing tests, they made reviewing this a lot easier).

nit, maybe you can link directly to that section? https://wiki.centos.org/Sources#Message_Broker_.28MQTT.29

typo: s/recieve/receive/.

so would C8S_GIT_BRANCH

what is package structure? Could it be documented as part of select_packages? That method mention returning a list of packages, here static field numbers are used, but I am failing to understand what those fields are.

We should use quay.io/centoshyperscale/centos:stream8 here.

Is this ever expected? Should it be logged so it can be troubleshooted?

I wished pagure allowed me to write multiple comments before updating the PR...

Just to set expectations, I will most likely not be able to review the whole diff in one go but will do my best to go through as much as possible.
I have never used MQTT, so excuse if my comments don't make sense at times :stuck_out_tongue_closed_eyes:
Some of the comments I made so far would be probably be better addressed in separate PRs so we can keep the discussion focused on the core business logic in this PR.

typo: establish(es?) a connection with the MQTT server and subscribes

On the other hand, it seems this callback only subscribes on connection success.

What happens if connection fails? Will it automatically retry?

what is the purpose of converting to a str? json accepts binary string just fine, f-formatted string can deal with the concatenation just fine too.
Given that python now has binary strings as first class citizen, we may want to avoid conversion unless strictly needed?

ok, I think that answers my previous question about the fields :)
The sqlite3.Row seems nifty to allow accessing fields https://docs.python.org/3/library/sqlite3.html#row-objects

Thanks @oidoming !

LGTM, sorry for the spam that inline commenting creates :(.

Thanks for adding unit tests as well as a test publisher. The README/workflow diagram was also very useful.

To re-iterate on a previous comment I made, pretty much all my comments don't need, and should not, be address in this PR, except maybe for the typos here and there.

I did not fully review db.py, but it would seem to me that conn would be better of be the first argument given that this will be invariably the first one to any of those functions. I find it odd to have it last, but this may just be nitpicking.
The use of sqlite3.Row may make the code that uses the results more readable, but on the other hand, there is only a few places where this is used.

I would favor using the logging module over print so it goes to stderr, can easily be formatted in a way that make the log lines more useful for troubleshooting (like datetimes, files/lines).... it would also make logging exceptions easier and with more context through the use of logging.exception for instance.

Having stuffs like PAGURE_REPO_API_URL... default to bogus URLs (something like https://localhost/foo/bar) and having it configurable to explicitly point to the prod URL may prevent rogue test polluting the dataset we will rely on?

I agree with basically all of @chantra's feedback, including the observation that none (except maybe the typos) need to be fixed here in this PR. But things like proper logging and use of sqlite3.Row are great readability/quality-of-life improvements for a future PR.

I know from talking to @oidoming that CS9 support was always intended as next-step functionality for a future PR, so I think that's the appropriate time to deal with making tag types configurable and whatnot.

We'll also need to manage the CentOS Steam 9 tags (I guess with a separate instance of this?) so you may wanna make these configurable.

About c9s packages, I'm confused, noticed that for c9s there is a GitLab repo and I can see that in kojihub the package builds source points to the GitLab URL (in koji mbox, for c8s packages, the URL is the git.centos.org one) but in git.centos.org there is a c9-beta branch (e.g., https://git.centos.org/rpms/rpm/tree/c9-beta), what is the difference? At the end GitLab and git.centos.org package versions are synchronized?

Is this ever expected? Should it be logged so it can be troubleshooted?

There are some hs packages that don’t have a c8s release tag (or c8s branch) in git.centos.org (for example, https://git.centos.org/rpms/btrfs-progs/releases), so it is omitted.

What happens if connection fails? Will it automatically retry?

loop_forever function automatically handles reconnecting https://www.eclipse.org/paho/index.php?page=clients/python/docs/index.php#:~:text=paho/temperature%22%2C%20temperature)-,loop_forever(),-loop_forever(timeout%3D1.0

but maybe we can have a more robust connection approach when connecting at the first time :)

what is the purpose of converting to a str? json accepts binary string just fine, f-formatted string can deal with the concatenation just fine too.
Given that python now has binary strings as first class citizen, we may want to avoid conversion unless strictly needed?

You are right, the conversion is not needed, I'll check this

Thank you all for your feedback. So yeah I'll correct the typos :smile: and address the other improvements in a future PR :D

We'll also need to manage the CentOS Steam 9 tags (I guess with a separate instance of this?) so you may wanna make these configurable.

About c9s packages, I'm confused, noticed that for c9s there is a GitLab repo and I can see that in kojihub the package builds source points to the GitLab URL (in koji mbox, for c8s packages, the URL is the git.centos.org one) but in git.centos.org there is a c9-beta branch (e.g., https://git.centos.org/rpms/rpm/tree/c9-beta), what is the difference? At the end GitLab and git.centos.org package versions are synchronized?

The source of truth for c9s is GitLab. The c9* branches on git.centos.org are for the RHEL source code drops (so the c9-beta branch is for the RHEL 9.0 Beta source, and presumably when RHEL 9.0 releases a c9 branch will show up). There's no syncronization between git.centos.org and GitLab that I know of.

1 new commit added

  • Change base image & correct typos.

1 new commit added

  • Add intel cbs tags

1 new commit added

  • Change pagure repo url to package-updates

working well on Openshift

Awesome, thank you. Let's get this merged for now, and we can iterate on it with further PRs.

Pull-Request has been merged by dcavalca

Metadata