docs: NO-JIRA: clarify serialization tag behaviour in api/AGENTS.md#8328
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: LGTM mode |
|
@enxebre: This pull request explicitly references no jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
added
the
jira/valid-reference
enxebre
force-pushed
the
enxebre/api-agents-serialization-tags
branch
from
af8e045 to
bad810b
Compare
enxebre
changed the title
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: enxebre 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 |
openshift-ci
Bot
added
approved
enxebre
force-pushed
the
enxebre/api-agents-serialization-tags
branch
from
bad810b to
add4f98
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited) Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe pull request updates 🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
enxebre
force-pushed
the
enxebre/api-agents-serialization-tags
branch
from
add4f98 to
21b69ae
Compare
enxebre
changed the title
| - **Downstream (authoritative):** [OpenShift API conventions](https://github.com/openshift/enhancements/blob/master/dev-guide/api-conventions.md) | ||
| - **Upstream (informational):** [Kubernetes API conventions](https://github.com/kubernetes/community/blob/main/contributors/devel/sig-architecture/api-conventions.md) |
There was a problem hiding this comment.
Both of these docs are huge, is telling the agent to read so much context going to cause context rot? Some of the serialization stuff you add here is already mentioned in these docs, but its in the ~middle. So I suspect the agent forgets it because we are giving it too much.
I think those docs need a lot of work to make them agent accessible
Worth teaching the agent to trust the linter more? And save it from having to consume any conventions docs that cannot be linted?
There was a problem hiding this comment.
Worth teaching the agent to trust the linter more? And save it from having to consume any conventions docs that cannot be linted?
I see those as different goals. We can and do enforce the agent building blocks to run the linter, we can do that more deterministically with git/cc hooks, whatever. What I'm after here is to improve the agent ability to reason about api machinery and understanding why the linter does what it does, not to enforce running the linter
I agree on the signal/noise potential problem, and this has not given good results so far, so let's change it.
| ### Serialization | ||
|
|
||
| - `omitempty` **does nothing for non-pointer structs.** Only `omitzero` correctly omits a struct field when it equals its zero value. This is a Go encoding/json behavior, not a Kubernetes convention. | ||
| - **Always set `omitempty` or `omitzero` on every field, regardless of whether it is `+required` or `+optional`.** These tags control serialization, not validation. `+required` is a schema constraint enforced at admission time; the serialization tag controls what goes on the wire. Without a tag, a zero-value field serializes as an explicit value (e.g., `"pullSecret": {"name": ""}`), which makes the API server unable to distinguish "not set" from "explicitly set to empty." This breaks defaulting, server-side apply field ownership, and strategic merge patch — all of which rely on field absence to mean "don't touch this." |
There was a problem hiding this comment.
Be explicit about which tags?
| - **Always set `omitempty` or `omitzero` on every field, regardless of whether it is `+required` or `+optional`.** These tags control serialization, not validation. `+required` is a schema constraint enforced at admission time; the serialization tag controls what goes on the wire. Without a tag, a zero-value field serializes as an explicit value (e.g., `"pullSecret": {"name": ""}`), which makes the API server unable to distinguish "not set" from "explicitly set to empty." This breaks defaulting, server-side apply field ownership, and strategic merge patch — all of which rely on field absence to mean "don't touch this." | |
| - **Always set `omitempty` or `omitzero` on every field, regardless of whether it is `+required` or `+optional`.** `omitempty`/`omitzero` tags control serialization, not validation. `+required` is a schema constraint enforced at admission time; the serialization tag controls what goes on the wire. Without a tag, a zero-value field serializes as an explicit value (e.g., `"pullSecret": {"name": ""}`), which makes the API server unable to distinguish "not set" from "explicitly set to empty." This breaks defaulting, server-side apply field ownership, and strategic merge patch — all of which rely on field absence to mean "don't touch this." |
The other element of this is that without omission, a structured client serializes the empty object as you've said, and this passes a +required check (based on key presence) without any check on the value. Most often folks add +required but expect that means non-empty, which it doesn't. So the API user can forget to add a value for the required field, it then passes the requiredness check, and the reader sees a required field with empty value which is unexpected
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #8328 +/- ##
=======================================
Coverage 36.08% 36.08%
=======================================
Files 767 767
Lines 93485 93485
=======================================
Hits 33737 33737
Misses 57041 57041
Partials 2707 2707 🚀 New features to boost your workflow:
|
Explain why omitempty/omitzero should be set on every field regardless of whether it is required or optional: the tag controls serialization behaviour (what goes on the wire), not validation (which is enforced at admission time via the schema). Without the tag, zero-value fields serialize as explicit values, breaking defaulting, server-side apply, and strategic merge patch. Add upstream Kubernetes API conventions link alongside the downstream OpenShift API conventions, with a note that downstream wins on conflicts. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
enxebre
force-pushed
the
enxebre/api-agents-serialization-tags
branch
from
21b69ae to
7adff1e
Compare
|
/lgtm |
|
Scheduling tests matching the |
Test Resultse2e-aks
e2e-aws
|
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
AI Test Failure AnalysisJob: Generated by hypershift-analyze-e2e-failure post-step using Claude claude-opus-4-6 |
|
@enxebre: The following tests failed, say
Full PR test history. Your PR dashboard. 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. I understand the commands that are listed here. |
|
Now I have a comprehensive picture. Let me generate the final report: Test Failure Analysis CompleteJob Information
Test Failure AnalysisErrorSummaryBoth jobs (AWS and Azure) experienced widespread, identical infrastructure-level failures unrelated to PR #8328 (a documentation-only change to Root CauseThe root cause is CI infrastructure instability affecting the management cluster's ability to provision and manage hosted clusters. The failure chain is:
The PR ( Recommendations
Evidence
|
4 participants
Summary
omitempty/omitzeroshould be set on every field regardless of+requiredor+optional: the tag controls serialization (what goes on the wire), not validation (enforced at admission via the schema). Without the tag, zero-value fields serialize as explicit values, breaking defaulting, server-side apply, and strategic merge patch.Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation