Show network name network events with podman -remote events #21409
Conversation
|
The word |
openshift-ci
bot
added
approved
rhatdan
force-pushed
the
events
branch
2 times, most recently
from
06cc30e to
98f1601
Compare
test/e2e/events_test.go
Outdated
| @@ -222,6 +222,38 @@ var _ = Describe("Podman events", func() { | |||
| Expect(result2.OutputToString()).To(ContainSubstring(fmt.Sprintf("pod_id=%s", id))) | |||
| }) | |||
|
|
|||
| It("podman events network connection", func() { | |||
| SkipIfRootless("Network creation not supported in rootless") | |||
There was a problem hiding this comment.
I don't think this is actually true?
There was a problem hiding this comment.
Error: "slirp4netns" is not supported: invalid network mode
There was a problem hiding this comment.
Yeah this should not be skipped, you must create the container with --network bridge to support connect/disconnect
|
Code LGTM but I have a question about the test |
|
@Luap99 PTAL and merge. |
test/e2e/events_test.go
Outdated
| @@ -222,6 +222,38 @@ var _ = Describe("Podman events", func() { | |||
| Expect(result2.OutputToString()).To(ContainSubstring(fmt.Sprintf("pod_id=%s", id))) | |||
| }) | |||
|
|
|||
| It("podman events network connection", func() { | |||
| SkipIfRootless("Network creation not supported in rootless") | |||
There was a problem hiding this comment.
Yeah this should not be skipped, you must create the container with --network bridge to support connect/disconnect
pkg/domain/entities/events.go
Outdated
| @@ -64,6 +68,7 @@ func ConvertToEntitiesEvent(e libpodEvents.Event) *types.Event { | |||
| attributes["containerExitCode"] = strconv.Itoa(*e.ContainerExitCode) | |||
| } | |||
| attributes["podId"] = e.PodID | |||
| attributes["network"] = e.Network | |||
There was a problem hiding this comment.
this should be wrapped in if e.Network != "" {} to only add this attribute if there is a network.
Note to self I have not checked how this is exposed in the docker compat API.
|
Dunno why (nor care really) the secret-scanner is mad here. Probably due to the merge commit, is my best guess. Probably safe to ignore. |
|
@Luap99 one more time... |
|
Still some not-addressed comments |
Fixes: containers#21311 Signed-off-by: Daniel J Walsh <[email protected]>
|
I addressed these comments before and must never have pushed them and then lost them. Good thing they are small. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: edsantiago, rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
rhatdan
added
lgtm
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
openshift-merge-bot
bot
merged commit 246831b
into
containers:main
stale-locking-app
bot
added
the
locked - please file new issue/PR
Fixes: #21311
Does this PR introduce a user-facing change?