Fix KObjSlice logging nil pointer entries as empty string#438
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: iasolanki The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
needs-triage
|
This issue is currently awaiting triage. If klog contributors determine this is a relevant issue, they will accept it by applying the The DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Welcome @iasolanki! |
k8s-ci-robot
added
size/M
|
Friendly ping for a kubernetes org member when someone has a minute — first time contributor here. CC @pohly @harshanarayana or any other reviewer on the OWNERS list. Thanks! |
kubernetes-prow
Bot
added
size/L
pohly
left a comment
pohly
left a comment
There was a problem hiding this comment.
You can squash directly into one commit, the force-push link from GitHub still shows the incremental changes.
iasolanki
force-pushed
the
fix-kobjslice-nil-entry
branch
from
0c90f10 to
1a6999a
Compare
| // and the structured output used by logr backends. Both must render a nil | ||
| // pointer entry as null rather than an empty ObjectRef. | ||
|
|
||
| func TestKObjSliceString(t *testing.T) { |
There was a problem hiding this comment.
Sorry, one more observation: both tests have exactly the same test cases, just different expected outcome for different operations on that input.
Can you consolidate into a single TestKObjSlice which has sub-tests "String" and "MarshalLog" for each test case?
There was a problem hiding this comment.
I have consolidated into a single TestKObjSlice with sub-tests, please take a look.
KObjSlice passes each slice entry to KObj, which converts a nil pointer
into an empty ObjectRef. That empty ObjectRef was then rendered as an
empty string (""), making a nil entry indistinguishable from an object
with an empty name.
A literal nil entry (untyped nil in the slice) was already handled and
logged as null, but a typed nil pointer (e.g. []*T{nil}) was not,
because it is a non-nil interface wrapping a nil pointer.
Detect nil pointer entries in both the structured (process) and text
(WriteText) code paths and log them as null, consistent with how an
untyped nil entry is already handled. For a non-nil entry the ObjectRef
is built inline rather than via KObj, so the added nil check does not
reflect on the value a second time.
Add a KObjSlice typed-nil-entry output test plus direct unit tests for
kobjSlice.String and kobjSlice.MarshalLog, the two paths that go through
process().
iasolanki
force-pushed
the
fix-kobjslice-nil-entry
branch
from
1a6999a to
5f193aa
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iasolanki, pohly The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
kubernetes-prow
Bot
added
approved
|
/assign @dims Please approve the workflows. |
|
@pohly Thanks for your approval. Just an heads-up, post-merge CI on main is red, but it's not from this change. Both Test and Run lint fail at the Set up job step, before any code is checked out or run: The actions actions/setup-go@v5 and actions/checkout@v4 are not allowed in kubernetes/klog because all actions must be pinned to a full-length commit SHA. The org-wide "actions must be pinned to a full commit SHA" policy is now being enforced, and the workflows still use version tags. |
|
@iasolanki do you mind creating a PR for pinning the version to SHA(s) please? |
|
@dims opened #439 to pin the actions to full-length commit SHAs, as requested. It covers both workflows:
Each SHA is pinned to the latest release of the major version already in use (no version bumps), with the tag kept in a trailing comment. Please take a look and since this touches |
4 participants
What this PR does / why we need it:
KObjSlicepasses each slice entry toKObj, which converts a nil pointer into an emptyObjectRef. That emptyObjectRefwas rendered as an empty string (""), making a nil entry indistinguishable from an object with an empty name. An untyped nil entry was already logged asnull, but a typed nil pointer (e.g.[]*T{nil}) was not, because it is a non-nil interface wrapping a nil pointer. This detects nil pointer entries in both the structured (process) and text (WriteText) paths and logs them asnull.Which issue(s) this PR fixes:
Fixes #409
Special notes for your reviewer:
Added a
KObjSlice typed nil entrytest case covering[]*kmeta{..., nil}. It produces the same expected output as the existingKObjSlice nil entrycase, so it reuses the existing expected-output mapping and no backend mappings changed.Release note: