fix: honor deprecated BuildRun status failedAt on conversion

[Ryntak94]

Changes

BuildRun conversion handled the deprecated .status.failedAt field in one direction only. ConvertTo deliberately mirrors FailureDetails.Location into failedAt so that users get some time to adopt failureDetails, but ConvertFrom never read it back. A v1alpha1 BuildRun that only reported .status.failedAt — which is what clients written before failureDetails existed do, and what this codebase itself emitted — converted to a v1beta1 object with no failure location at all, and the loss was silent.

ConvertFrom now falls back to the deprecated failedAt for the failure location. failureDetails stays authoritative: failedAt is only consulted when failureDetails carries no location, so it can never override the newer field.

Only the location is recovered. failureDetails.reason and failureDetails.message are left unset, because a v1alpha1 object that reports failedAt on its own carries no such data and synthesising values would be worse than leaving them empty. Whether those two fields should be required rather than optional is a status schema question that is deliberately out of scope here; it is tracked separately in #2308.

Related Issue

Fixes #2304

Type of PR

/kind bug

Submitter Checklist

• Includes tests if functionality changed/was added
• Includes docs if changes are user-facing — no API surface change; this makes an already-deprecated field round-trip instead of being dropped
• Kind label has been set
• Release notes block has been filled in, or marked NONE

Release Notes

Fixed BuildRun conversion ignoring the deprecated `.status.failedAt` field when converting from v1alpha1. The failure location is now carried into `.status.failureDetails.location` when `failureDetails` does not already provide one, so failure information from older clients is no longer lost.

[openshift-ci]

[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 irvingmg for approval. For more information see the 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

[Ryntak94]

Note on the commit list: this branch is stacked on #2305, so the diff here currently includes that PR's commit as well.

• 893c1af2 — belongs to fix: preserve BuildRun output insecure and timestamp on conversion #2305 (BuildRun .spec.output.insecure / .spec.output.timestamp)
• 797572c5 — the actual content of this PR (deprecated .status.failedAt)

Both changes touch pkg/apis/build/v1beta1/buildrun_conversion.go, in different places, which is why I stacked them rather than developing them independently and hitting a conflict. 797572c5 reviews cleanly on its own if that is easier.

Once #2305 merges, this will rebase down to the single commit. Happy to rebase onto main sooner instead if you would rather the two PRs were fully independent, or to close this and re-open it after #2305 lands — whichever suits your review order.

[SaschaSchwarze0]

Rebased ^^

[SaschaSchwarze0]

The idea of the FailureDetails was to have always a reason and a message. I do not know why we missed to annotate the two structs and why they have omitempty for reason and message. Our usual guidance is that required fields are not a pointer (is the case for them) and then omitempty also makes no sense. But it seems that we have not really been consistent in status types, should be better in spec types.

So, not sure if we want to add conversion code that builds FailureDetails where fields one could consider required, are not set.

[kaizakin]

old alpha objects can only preserve the failure location, and i think dropping it would be real data loss.

I do agree the status model is a bit inconsistent, especially around whether FailureDetails.reason and message are semantically required. my suggestion is to keep this PR scoped to backward compatibility and track the status-schema cleanup in a separate follow-up issue.

[kaizakin]

@SaschaSchwarze0 I've created an issue for this #2308

[Ryntak94]

Thanks @kaizakin, that matches what I had in mind, so I've kept this PR scoped to backward compatibility.

To be explicit about what the conversion does: ConvertFrom only recovers failureDetails.location from the deprecated failedAt, and only when failureDetails does not already carry a location, so it can never override the newer field. reason and message are left untouched — a v1alpha1 object that reports failedAt on its own has no such data, and synthesising values would be worse than leaving them empty. No API types change here.

I've pushed a commit that spells that out in a comment at the conversion site and pointed it at #2308, and updated the PR description to say the required-vs-optional question on FailureDetails.reason/message is out of scope here and tracked in that issue.

@SaschaSchwarze0 does that address your concern? Happy to adjust if you'd rather the fallback not construct a FailureDetails at all when only failedAt is present.

[Ryntak94]

Hey @kaizakin and @SaschaSchwarze0 I just wanted to bump this one since it seems like it's on the verge of being merged :)

[kaizakin]

Let's listen @qu1queee 's opinion on this one :)

[kaizakin]

Hi @Ryntak94 this PR is taking more time for approval because we want to hear opinion from other maintainers.

can you come to tomorrow's community meeting if you have time?
maybe we could discuss more.

meeting minutes: https://docs.google.com/document/d/1o0wdu6njw3mgVXXRHoi64lhaokLcIV2H0X5o8DoWKAM/edit?tab=t.0#heading=h.y6cwrxw5b3qq

[qu1queee]

I'd second Sascha's point. A non-nil br.Status.FailureDetails is meant to mean reason and message are present; in the conversion case they would not be. I also would not expect users to still have a BuildRun old enough for that gap (failedAt only, no failureDetails). There is no technical reason to block the conversion. I'd leave the product call to consensus in the community meeting.