avoid duplicate error event #1655
Conversation
k8s-ci-robot
added
the
do-not-merge/hold
✅ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jichenjc 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 |
k8s-ci-robot
added
approved
Good point. We should probably pull both events out of getOrCreateTrunk and generate them in the caller. Also, that error message should use |
|
there's only one usage, so update directly in the function... |
| if portOpts.Trunk != nil && *portOpts.Trunk { | ||
| trunk, err := s.getOrCreateTrunk(eventObject, clusterName, port.Name, port.ID) | ||
| if err != nil { | ||
| record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", portName, err) |
There was a problem hiding this comment.
I was thinking keep this one and remove the events in getOrCreateTrunk()? By emitting the event at both error points we're no longer avoiding duplicating the error event.
There was a problem hiding this comment.
Yeah, I'd probably go with what Matt says here, but no strong preference.
Signed-off-by: jichen <[email protected]>
|
/test pull-cluster-api-provider-openstack-e2e-test |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
/lgtm |
|
/hold cancel |
k8s-ci-robot
removed
the
do-not-merge/hold
4 participants
What this PR does / why we need it:
this is from code read when doing other PR
seems we have event https://github.com/kubernetes-sigs/cluster-api-provider-openstack/blob/main/pkg/cloud/services/networking/trunk.go#L73 so no need duplicate event to confuse admin
Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)format, will close the issue(s) when PR gets merged):Fixes #
Special notes for your reviewer:
TODOs:
/hold