Support artifacts emitting events#28662
Conversation
|
@nimdrak (not a review) rebase on main to get past Mac OS arm64 issue. |
37bb976 to
e3766d1
Compare
e3766d1 to
fab2171
Compare
fab2171 to
44a5293
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
47ec1f1 to
a43532c
Compare
|
I checked it works properly in Ubuntu 24.04.4 LTS. Detailsartifact events logging by journald
artifact events logging by file
|
|
I think the failed remote tests are unrelated to this PR. |
This is your new test so they cannot be unrelated. I think is the main problem via this "broken" multiple channel event architecture and why we need the interface... |
a43532c to
1c97af3
Compare
You're right, the timing issue between the service and the remote command was the culprit. I've applied Regarding the 'broken' event architecture, I agree that a proper interface is the right way to go. I’d like to be part of that refactoring process. If you have a specific design in mind or an existing issue for it, please let me know. |
|
|
||
| eventChannel := store.EventChannel() | ||
|
|
||
| go func() { |
There was a problem hiding this comment.
This looks like it could be refactored to be shared between this and libimageEvents
There was a problem hiding this comment.
Good point! How about this approach? fb462d9
I deduplicated the common logic by extracting it into a generic function in events.go.
50f427c to
fb462d9
Compare
|
The recent test failures were triggered by my changes. I believe this comes from a timing issue during the initialization between To resolve this without a massive overhaul of the current architecture, I considered three options: Rather than refactoring the entire channel architecture in this PR (as I understand a larger refactoring is planned soon), I opted for (1) as the simplest immediate fix. I have updated the code accordingly. |
fb462d9 to
b03c100
Compare
|
testing-farm:fedora-44-x86_64:podman-fedora failed but it said
|
|
We really need to get those marked as allowed-failure |
|
LGTM |
|
please squash the commits, docs/test and so on should be part of the same commit, it also not great for reviews to have a refactor commit last when I already looked at the now again removed code by that refactor. Normally the refactor should be the first commit but I am fine if you also sqash down everything into one commit. |
- libpod/events.go & libpod/runtime.go: Added the `Artifact` event type. Refactored and deduplicated event forwarding logic by introducing `spawnEventForwarder[T any]`, replacing separate goroutine loops for images and artifacts. Implemented graceful shutdown and resolved eventer initialization race conditions. - libpod/events: Implemented event filtering by name/ID, updated journald and logfile readers/writers for artifact events, and added `Artifact` to `ToHumanReadable` formatting. - cmd/podman: Added shell auto-completion for `artifact=` and `type=artifact` filters. - docs/test: Documented the `artifact` event type, statuses, and filters in `podman-events.1.md`. Added an end-to-end test in `events_test.go` to verify event emissions. Signed-off-by: Byounguk Lee <nimdrak@gmail.com>
b03c100 to
a82cdef
Compare
|
Sorry for the confusion during the review. I agree that the refactor should have come first and that docs/tests belong in the same commit. I've squashed everything into a single commit now. I'll structure the commits more carefully next time. Thanks for the feedback. |
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?
fix #27260