fedora-infra / fedora-messaging

A library for sending AMQP messages with JSON schema in Fedora infrastructure
GNU General Public License v2.0
53 stars 52 forks source link

Added a replay command #334

Closed Freedisch closed 6 months ago

Freedisch commented 7 months ago

Description of changes

Freedisch commented 7 months ago

I'm not quite sure of the reason behind the failing of these CI tests, but I'm exploring it to see what's causing it๐Ÿค”

gridhead commented 7 months ago

The tests might not necessarily fail from the changes that you made as they are installability tests but try adjusting the version requirement for the pytz dependency. From https://github.com/fedora-infra/fedora-messaging/pull/334/checks?check_run_id=22273594026, I got to the log dumps present at https://download.copr.fedorainfracloud.org/results/packit/fedora-infra-fedora-messaging-334/fedora-40-x86_64/07105096-fedora-messaging/builder-live.log.gz and here is an excerpt of what causes the failures.

INFO: Going to install missing dynamic buildrequires
Updating and loading repositories:
 updates                                100% | 803.1 KiB/s |  23.3 KiB |  00m00s
 fedora                                 100% | 691.1 KiB/s |  20.0 KiB |  00m00s
 Copr repository                        100% |  44.4 KiB/s |   1.5 KiB |  00m00s
Repositories loaded.
Failed to resolve the transaction:
Package "pyproject-rpm-macros-1.12.0-1.fc40.noarch" is already installed.
Package "python3-devel-3.12.2-1.fc40.x86_64" is already installed.
Package "python3-sphinx-1:7.2.6-6.fc40.noarch" is already installed.
Package "python3-packaging-23.2-4.fc40.noarch" is already installed.
Package "python3-pip-23.3.2-1.fc40.noarch" is already installed.
Package "python3-poetry-core-1.8.1-3.fc40.noarch" is already installed.
Package "systemd-rpm-macros-255.3-1.fc40.noarch" is already installed.
Problem: nothing provides requested (python3dist(pytz) < 2024~~ with python3dist(pytz) >= 2023.3)
mergify[bot] commented 7 months ago

โš ๏ธ The sha of the head commit of this PR conflicts with #335. Mergify cannot evaluate rules on this PR. โš ๏ธ

gridhead commented 7 months ago

This should do it.

Also as a rule of thumb - please use the simple present verb in your commit messages.

Commit messages describe what a commit does to a repository and not what you did in a commit.

Freedisch commented 7 months ago

Commit messages describe what a commit does to a repository and not what you did in a commit.

Got it, ๐Ÿ™Œ๐Ÿพ

abompard commented 7 months ago

One more thing: please allow the user to specify the location of the datagrepper instance (defaulting to apps.fedoraproject.org/datagrepper) because we may want to replay messages from the staging environment.

Freedisch commented 7 months ago

@abompard, I see it makes sense, I'm on it

Freedisch commented 7 months ago

@abompard I have added the support to user to specify a custom URL

Freedisch commented 6 months ago

Another minor thing. And please don't forget the unit tests! I can help with that if you're unfamiliar with unit testing.

I'm familiar with unit tests, I will check the existing ones and follow the same workflow.

Freedisch commented 6 months ago

Test cases results:

Screenshot from 2024-03-15 20-50-05

Screenshot from 2024-03-15 20-49-03

abompard commented 6 months ago

OK great! Only the --datagrepper-url option name to fix! :-)

Freedisch commented 6 months ago

OK great! Only the --datagrepper-url option name to fix! :-)

Actually, I thought I fixed it already ๐Ÿ˜…

abompard commented 6 months ago

Ah, yeah, the unit test needs to be adjusted as well.

abompard commented 6 months ago

OK thanks! Could you rebase and solve the conflicts? If you have any issues doing that please ask.

Freedisch commented 6 months ago

OK thanks! Could you rebase and solve the conflicts? If you have any issues doing that please ask.

I was wondering also If I should squash all these commits in one?

abompard commented 6 months ago

I was wondering also If I should squash all these commits in one?

Yeah that would be preferable.

abompard commented 6 months ago

Merged, thanks again for your contribution.