Conversation
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1565 (linux/amd64, linux/arm64) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1565", name: che-dashboard}]}}]" |
After a workspace rename, the dashboard used the display name (from the kubernetes.io/metadata.name label) instead of the immutable metadata.name for backup status API calls, causing 404 errors. This commit: - Adds a resourceName getter to the Workspace adapter that returns metadata.name, and updates backup status callers to use it - Auto-generates metadata.name with a random suffix during restore (matching factory workspace creation) and stores the user-typed name as the display name label, eliminating false conflict errors - Includes ADR-001 documenting the architectural decision Fixes: https://redhat.atlassian.net/browse/CRW-10477 Assisted-by: Claude Opus 4.6 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1565 (linux/amd64, linux/arm64) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1565", name: che-dashboard}]}}]" |
There was a problem hiding this comment.
To double check, was this doc meant to be committed to the repo? (I mean, should this file be merged to the main branch with this PR?)
There was a problem hiding this comment.
Yes, I committed it intentionally. The restore flow change is a non-trivial decision with trade-offs, so I'd like to keep ADRs for such changes in the repo, so the reasoning is next to the code itself. I believe this will help both humans and AI agents providing better code.
There was a problem hiding this comment.
We discussed the same matter with @rohanKanojia here: devfile/devworkspace-operator#1631 (review)
We agreed on having ADRs under the adr/ directory, in case of the dashboard - in the root of the package.
There was a problem hiding this comment.
@akurinnoy do I understand correctly that ADR files should be maintained and updated whenever the logic changes?
Also, I’m not sure whether we should reference the downstream issue here
There was a problem hiding this comment.
@akurinnoy Placing ADRs under packages/dashboard-frontend/adr/ is OK for me.
However, there is an important concern about treating ADRs as permanent, append-only artifacts.
ADR files like adr-001-auto-generate-resource-name-on-restore.md describe a decision that was correct at a point in time. When the underlying logic changes, the ADR can become stale or misleading. Unlike code, outdated ADRs don't produce compiler errors — they silently rot.
The risk: Without explicit rules for updating or retiring ADRs, the adr/ directory will accumulate documents that no longer reflect reality, which hurts rather than helps future contributors and AI agents.
What's needed before committing to this pattern:
- A policy for when an ADR should be updated vs. superseded (a new ADR that marks the old one as replaced).
- A policy for when an ADR should be removed (e.g., if the feature it describes is deleted entirely).
- Ideally, a lint or review checklist item: "Does this change affect an existing ADR?"
Without these rules, ADRs become archaeology rather than documentation.
Proposed First Step: Add a /docs Directory with a Project Snapshot
Before expanding the ADR convention further, a more immediately valuable step is to establish a top-level docs/ directory (or openspec/docs/) containing a snapshot description of the project — what it does, its key architectural boundaries, package structure, and major design decisions in effect today.
Why this matters:
- It gives human contributors and AI agents a fast-path to understanding the project without reading all the code.
- It saves time and LLM tokens on every AI-assisted task — agents can orient themselves from the snapshot rather than exploring the codebase from scratch each time.
- A single well-maintained snapshot is easier to keep accurate than a growing collection of individual ADRs.
The snapshot should be treated as a living document: updated when significant decisions change, not as a historical record. This is different from ADRs, which record the history of a decision.
There was a problem hiding this comment.
@svor thanks for the feedback
do I understand correctly that ADR files should be maintained and updated whenever the logic changes?
The standard ADR practice is append-only: old ADRs are not edited, instead you write a new one that supersedes the previous (with Status: Superseded by ADR-XXX). If the feature is removed entirely, you mark it Status: Deprecated. The idea is that ADRs are a decision log, not a living document. Even a deprecated ADR still has value - it explains why the approach was tried and why it was abandoned, so future contributors don't repeat the same mistakes.
I'm not sure whether we should reference the downstream issue here
Good point. I'll replace the Jira link with a reference to this PR.
There was a problem hiding this comment.
@olexii4 thanks for the thorough feedback
Regarding the ADR lifecycle - the standard ADR convention already covers this: ADRs are append-only, you don't edit old ones. If the logic changes, you write a new ADR that supersedes the previous one (Status: Superseded by ADR-XXX). If a feature is removed, you mark it Status: Deprecated. I can add a short README.md in the adr/ directory explaining this convention.
On the "silently rot" concern - ADRs don't describe current state, they describe why a decision was made at that point in time. A superseded or deprecated ADR is not stale, it's still doing its job: it explains why the approach was tried and why it was later changed or abandoned. This prevents future contributors from repeating the same mistakes.
Regarding the project snapshot in docs/ - interesting idea, but I think AGENTS.md in the repo root already serves this purpose. Happy to discuss expanding it, but that's a separate effort and shouldn't block shipping ADRs.
Agreed with the team to keep ADRs under adr/ in the package root. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Oleksii Kurinnyi <okurinny@redhat.com>
|
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1565 (linux/amd64, linux/arm64) kubectl patch commandkubectl patch -n eclipse-che "checluster/eclipse-che" --type=json -p="[{"op": "replace", "path": "/spec/components/dashboard/deployment", "value": {containers: [{image: "quay.io/eclipse/che-dashboard:pr-1565", name: che-dashboard}]}}]" |
|
I successfully verified this PR on a ROSA cluster running Eclipse Che 7.117.0 with global backups enabled. First, I confirmed the backup tracking fix by renaming a workspace from original-name to renamed-workspace; the dashboard correctly mapped the display name to the immutable Kubernetes ID and successfully displayed its backup status. Second, I verified the restore lifecycle by deploying a clone named restored-workspace. After sequentially stopping them to release storage locks, both instances independently achieved a clean Success backup state. Both the data mapping and resource-decoupling fixes are functional. LGTM |
|
/retest |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akurinnoy, olexii4, svor 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 |
|
@akurinnoy: The following test 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. |
What does this PR do?
This PR fixes backup status fetching and restore conflict detection after workspace rename by adding a
resourceNamegetter to theWorkspaceadapter. Additionally, decouples the user-typed workspace name frommetadata.nameduring restore by auto-generating the resource name with a random suffix (matching the factory workspace creation pattern) and storing the user input as the display name label.See
packages/dashboard-frontend/docs/adr-001-auto-generate-resource-name-on-restore.mdfor the architectural decision record.Screenshot/screencast of this PR
What issues does this PR fix or reference?
Fixes https://redhat.atlassian.net/browse/CRW-10477
Is it tested? How?
backup-statusAPI call uses the original K8s resource name, not the renamed displayname
"Never"), not an error
display name and a suffixed
metadata.name(visible viakubectl get devworkspaces)produces a false conflict error
Release Notes
Fixed backup status and restore failing after workspace rename. Restore now auto-generates the K8s resource name, consistent with workspace creation.
Docs PR