Return ConditionUnknown from Unknown()

[thc1006]

This patch makes Unknown() return metav1.ConditionUnknown in both cfg/v1alpha1 and infra/v1alpha1. It previously returned metav1.ConditionFalse, which is the status Failed() returns a few lines below, so the two were distinguishable only by their Reason and a consumer switching on Status read "not known yet" as "definitely failed". metav1.ConditionUnknown exists for exactly the state this constructor names.

The generated schema already promises the third state

Quoting config/crd/bases/infra.nephio.org_repositories.yaml, and the other manifests whose status is built from these packages:

status:
  description: status of the condition, one of True, False, Unknown.
  enum:
  - "True"
  - "False"
  - Unknown
  type: string

The published contract is a tri-state, while the one constructor meant to produce the third value could not emit it. ConditionReasonUnknown was declared and used; metav1.ConditionUnknown appeared nowhere in either package.

This is not a regression. infra/v1alpha1/condition.go was introduced by 3316d8b (#23) on 2023-05-11 and the line has not been modified since. cfg/v1alpha1/condition.go is a later whole-file copy of it, added by ceb0d10 (#46) on 2023-06-15.

Why both packages

An earlier revision of this PR fixed infra/v1alpha1 only. That was wrong, and I have corrected it.

The two packages export a helper of the same name with the same documented contract. Fixing one would make Unknown().Status depend on which package a caller imports, which is a worse state than the uniform defect it replaces — and Network appears to be moving from infra.nephio.org to config.nephio.org, so the migration target would have been the broken one. cfg/v1alpha1 has no importers I could find and infra/v1alpha1 has many, but both are published API.

Impact

No in-tree or publicly indexed caller of Unknown() was found — not in this repository, in nephio-project/nephio, in nephio-project/oai, or among the public importers of this module. That is deliberately narrower than saying there is no impact: this changes the observable behaviour of an exported function in a published module, and unindexed consumers may exist.

Worth flagging separately: the pinned nokia/k8s-ipam dependency carries the same defect in its own copy of this file, and unlike here it has roughly twenty-seven live callers across six reconcilers that persist the result through Status().Update(). So resources written by that controller do carry status: False with reason: Unknown today. That is not affected by this PR and should be raised upstream, but it means any ecosystem-wide claim about serialised conditions would be wrong.

About the SonarCloud result

The gate is green, but not because the duplication went away. On this revision Sonar reports 14 new lines, 14 duplicated new lines, and a density of 100%, against a threshold of 3. It passes only because it returns ignoredConditions: true: per the SonarQube Cloud documentation, "Conditions on Coverage and Duplicated lines are not calculated when there are fewer than 20 lines."

cfg/v1alpha1/condition.go and infra/v1alpha1/condition.go are whole-file duplicates of each other, as are their tests, so anything added to both lands inside a matching block. An earlier revision added a thirteen-line testify-based test to each file, came to 30 new lines, lost the exemption and failed the gate outright. The density is the same either way; only the line count changed.

I would rather state that than let a green check imply the duplication was resolved. Deduplicating the two packages is the real fix and is out of scope here.

What is deliberately left alone

ConditionedStatus.GetCondition() collapses the same tri-state in the other direction: it synthesises ConditionFalse when no condition of the requested type exists, while its doc comment says it "returns nil". Whether absence should read as unknown is a separate decision about public API semantics.

The whole-file duplication between the two packages, and the same defect in the upstream copies this file descends from, are likewise out of scope.

Proof

The new test asserts the status alone, which is the field this change alters:

func TestUnknown(t *testing.T) {
	if got := Unknown().Status; got != metav1.ConditionUnknown {
		t.Errorf("Unknown().Status = %q, want %q", got, metav1.ConditionUnknown)
	}
}

It was run with -v to confirm it executes rather than silently matching nothing, and checked against the unfixed code in both packages. Reverting either Status line fails that package's test:

--- FAIL: TestUnknown (0.00s)
    condition_test.go:224: Unknown().Status = "False", want "Unknown"

go test ./... -count=1 (three consecutive runs), go test -race ./cfg/v1alpha1 ./infra/v1alpha1 -count=10, TestUnknown -count=100, go vet ./..., golangci-lint run ./... (0 issues), gosec ./... (0 issues), gofmt -l, and git diff --check are all clean. make unit and make lint shell out to a container runtime that is not available here, so the commands from their non-container branches were run directly.

Note that .golangci.yml sets skip-files: ".*_test.go$", so the added test is not linted or gofmt-checked by CI; gofmt -l was run on it directly.

A note on the test placement

The regression check could instead be folded into the existing DifferentStatus case in TestConditionEqual, by setting a: Unknown() against a literal ConditionFalse condition. I built that variant from 4827d95 and measured it: it is +6/-6 across the same four files, it passes, and reverting either production line does fail that package's test. So it works, and it is eight lines lighter.

I kept the standalone test for one reason that survived the comparison. Both variants sit at 100% duplicated new lines and both are green only through the under-20-lines exemption, so neither actually reduces duplication — but they differ in what a future failure tells you. This version reports Unknown().Status = "False", want "Unknown", which names the defect. The folded version reports TestConditionEqual/DifferentStatus, which points a reader at Equal() rather than at the constructor that regressed, and it converts a case that currently exercises Equal() with plain True/False fixtures into one that also depends on Unknown().

That is a judgement call rather than a clear win, and the eight lines are a real cost. Happy to switch if you prefer the shorter diff.

[nephio-prow]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign kispaljr for approval by writing /assign @kispaljr in a comment. For more information see the Kubernetes Code Review Process.

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

Details

Needs approval from an approver in each of these files:

• OWNERS

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

[nephio-prow]

Hi @thc1006. Thanks for your PR.

I'm waiting for a nephio-project member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

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 kubernetes/test-infra repository.

[thc1006]

Both API packages now return ConditionUnknown. An earlier revision of this PR fixed infra/v1alpha1 only, which would have made Unknown().Status depend on which package a caller imports — worse than the uniform defect it replaced — so cfg/v1alpha1 is corrected too.

I have also corrected several factual errors in the description: the implementation being changed came from 3316d8b (#23) rather than ceb0d10 (which added the later cfg copy), Unknown() was already at 100% statement coverage on the base revision so the new assertion adds semantic rather than execution coverage, and the impact claim is narrowed to "no in-tree or publicly indexed caller was found" rather than no behaviour change.

The full Go suite, a race run, vet, golangci-lint, gosec, gofmt and diff checks are clean locally. Sonar is green, though I have noted in the description that this is because it skips duplication and coverage conditions below 20 new lines, not because the two packages stopped being whole-file duplicates.

Could a project member please /ok-to-test so the Prow presubmits can run?

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
100.0% Duplication on New Code

See analysis details on SonarQube Cloud