🐛 fix: omit PlacementDecision score field when zero-valued

[mikeshng]

Summary

Add omitzero json tag to ClusterDecision.Score so that the default zero value is not serialized to JSON. The field remains int64 (non-pointer) and is only included when explicitly set to a non-zero value.

Related issue(s)

Fixes #

Summary by CodeRabbit

• Refactor
  • Optimized JSON serialization to omit zero score values in cluster decision responses, reducing payload size.

[coderabbitai]

Walkthrough

Modified the JSON serialization tag for the Score field in the ClusterDecision struct from json:"score" to json:"score,omitzero", causing the score field to be omitted from JSON output when it contains a zero value.

Changes

Cohort / File(s)	Summary
ClusterDecision JSON Serialization cluster/v1beta1/types_placementdecision.go	Changed ClusterDecision.Score JSON tag to include omitzero directive, which will exclude the score field from JSON encoding when its value is zero.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

• ✨ Fields for placement score visibility feature #428: Added the ClusterDecision.Score field that this PR modifies with the omitzero JSON tag.

Suggested labels

lgtm

Suggested reviewers

• qiujian16

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 inconclusive)

Check name	Status	Explanation	Resolution
Description check	❓ Inconclusive	PR description provides a clear summary but does not follow the required template structure with categorization emoji and formatted sections.	Add a PR title prefix (e.g., 🐛 or ✨) and ensure the description follows the template format with proper section headings as specified in CONTRIBUTING guidelines.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check	✅ Passed	The title accurately reflects the main change: adding omitzero to the Score JSON tag to omit zero-valued fields, which is the primary modification in the changeset.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[mikeshng]

Discussed with original enhancement author @bhperry and he agreed this is the right approach.

/assign @qiujian16

[bhperry]

/lgtm

[bhperry]

I misunderstood the problem. Does this actually fix argoproj/argo-cd#27264 ? It seems like argo actually needs to handle non-strings or it will still break once the PR to set thr scores merges.

Score should always be set once the feature is complete.

[bhperry]

@mikeshng would it be enough to just set omitempty to make argocd not break while they fix the underlying problem?

I think having it be a pointer is confusing for the api, when the intention is that it will always be set to a number.

[mikeshng]

This doesn't directly fix the argo-cd issue. The argo-cd fix is still required. This current API change PR gives us enough time to address it properly.

[qiujian16]

can we set omitzero instead? will it be good enough for serielization？

[mikeshng]

@bhperry @qiujian16 thank you for the quick reviews. I've done the changes as you both suggested. Thanks!

[qiujian16]

think about it more. I wonder if "not set" is meaningful when the feature is enabled. I guess not, when feature is enabled and score is zero, it does not mean the scheduler does not set the value, but just indicate the score is zero.

[mikeshng]

@bhperry , could you please see @qiujian16 's comment? I think both me and him prefer if we use a pointer here. Thanks.

[bhperry]

Once the feature in OCM is merged (I will rebase the PR tomorrow) score will never be "not set". When cluster decision is first created it has its initial score. That is why I don't think a pointer makes sense -- it will always be set to an integer, so it's an extra pointer dereference + nil check for no reason.

In my opinion the best way forward would be:

• Set omitzero as a temp fix for argo
• Wait for argo fix to be merged, then merge the scoring feature in OCM
• Remove omitzero from the API (as you mentioned @qiujian16 a score of zero means explicitly set to zero, so it should be included in serialized output)

[qiujian16]

ok makes sense to me.

/approve
/lgtm

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mikeshng, qiujian16

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

• OWNERS [qiujian16]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[bhperry]

/lgtm