Kube token sync v2#1492
Conversation
|
Click here to review and test in web IDE: |
|
Hi @batleforc. Thanks for your PR. I'm waiting for a eclipse-che member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. 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. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1492 +/- ##
==========================================
+ Coverage 92.37% 92.42% +0.05%
==========================================
Files 587 596 +9
Lines 60485 61138 +653
Branches 4688 4728 +40
==========================================
+ Hits 55870 56504 +634
- Misses 4556 4575 +19
Partials 59 59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@batleforc hello, thank you for the contribution. Could you please clarify the UX e.g. when exactly user needs to do this procedure manually? I guess it is related to the short-lived tokens, but afaik for OpenShift it is 24h by default |
|
Hi @ibuziuk In my case, the token should at most live 6H (set in the different OIDC providers) and sometimes the end of the token's life comes in the middle of a debug process or a long-lived process so restarting the full app ends up being a headache more than anything. And extending the token's life is not possible. |
|
@batleforc thank you for clarification, but I'm a bit concerned about exposing this capability on the UI since it is pretty niche. Could you clarify how you are currently updating the kubeconfig? is it |
|
@ibuziuk in the PR yes i call this function https://github.com/batleforc/che-dashboard/blob/main/packages/dashboard-frontend/src/services/backend-client/devWorkspaceApi.ts#L126 that is directly exposed in the fronted. At the moment my user or i are annoyed by it and redo the kube oidc login full process or wait for some more time to restart the container. |
@batleforc Hello, need to increase test coverage for your changes from 55% to 92% (add some tests). |
|
@batleforc thank you for the details and follow-up. I believe we need some option to re-inject the token without the direct user interaction and exposing internals on UI. |
|
Shoud i finish adding more test or not ? @ibuziuk |
@tolusha @olexii4 @batleforc tbh, I would only add this action to VS Code, without adding new menu item to the user dashboard @TheChosenMok please, review, wdyt ^ |
|
@ibuziuk one of the reasons has to why I chose to put it in the dashboard was to not have to trigger a plugin for the different kinds of IDE (VsCode, VsCode Desktop, JetBrains, WebShell, and existing and future ones) |
|
@batleforc, that is a fair take. For me, the UX is questionable, though, when the user is expected to go to the dashboard from the workspace and manually trigger the injection. |
|
@ibuziuk in any case he has to, to re-login to the dashboard |
|
It's a neat idea, but it's a bit too niche to add it to the main dashboard as it might cause some clutter. That being said what what are yall's thoughts on adding it to the workspace details tab (either within another tab or in the overview). In the future we may have more advanced configs like this so it may not be the only one and having a place to put them would be good. I think a new a tab called "Advanced" or something like that would be approriate. 
|
|
Since DevSpaces 3.28 lot's of user end's up with the token not injected after a restart in the workspace and we rely a lot on it to start/stop/interact with the sidecar. |
|
@batleforc |
|
So I moved the button from the drop-down to the new "Advanced" tab, and relied on the toast to ask the user to reload the IDE tab in order for the extensions to start working again. In the advanced tab i made it to allow an eassy add of other kind of action based on a grid. For exemple if in the future we need to trigger an action to reset the ide (wipe /idea-server then restart) we could easily had it. FYI, We had a need to reset the ide in case of user that didn't had the time to commit there work and had a problem with the IDE like if you start with a JetBrains IDE and end's up out of space (heavy lib or java project) the Workspace corrupt itself sometime. If wanted i could also add this kind of button if needed. |
olexii4
left a comment
olexii4
left a comment
There was a problem hiding this comment.
@batleforc great job getting this to the Advanced tab design and raising coverage to 99.7% — the
Redux slice structure is clean and the test suite is thorough. A few things to fix before this
can merge.
Blocking
1. refreshKubeconfig action does not guard against a non-running workspace
src/store/Kubeconfig/actions.ts — injectKubeConfig and podmanLogin are called with
whatever namespace and devworkspaceId are passed in, but nothing prevents the caller from
dispatching this action for a stopped or starting workspace. The component disables the button
when !workspace.isRunning, but that guard is only in the UI layer. Add a phase check in the
action itself so the backend is never called with a workspace that cannot accept the injection:
export const actionCreators = {
refreshKubeconfig:
(namespace: string, devworkspaceId: string, phase: string): AppThunk =>
async dispatch => {
if (phase !== DevWorkspaceStatus.RUNNING) {
throw new Error('Cannot refresh kubeconfig: workspace is not running.');
}
dispatch(kubeconfigRefreshRequestAction());
// ...
},
};Add a test case for this in actions.spec.ts:
it('should throw without dispatching any action when workspace is not running', async () => {
await expect(
store.dispatch(actionCreators.refreshKubeconfig('ns', 'id', DevWorkspaceStatus.STOPPED)),
).rejects.toThrow('workspace is not running');
expect(injectKubeConfig).not.toHaveBeenCalled();
expect(store.getActions()).toHaveLength(0);
});2. workspace.id is passed without a null guard
RefreshKubeconfig/index.tsx:219 — workspace.id is status?.devworkspaceId, which can be
undefined for a freshly created workspace whose status has not been populated yet. The button is
disabled when !workspace.isRunning, but isRunning only checks status.phase; devworkspaceId
can still be absent on the first status update. If undefined reaches the backend, it returns an
opaque 400. Add a guard before dispatching:
private async handleRefreshKubeconfig(): Promise<void> {
const { workspace } = this.props;
const { namespace, id: devworkspaceId } = workspace;
if (!devworkspaceId) {
this.showAlert(AlertVariant.danger, 'Workspace ID is not available. Try again in a moment.');
return;
}
try {
await this.props.refreshKubeconfig(namespace, devworkspaceId);
// ...3. Add Assisted-by: trailer to commit messages
The // Generated by Claude Sonnet 4.6 comments in the source files are correct and required by
redhat-compliance-and-responsible-ai.md. Keep them as-is.
The missing piece is the commit message trailer. Each commit that contains AI-generated code must
also carry:
Assisted-by: Claude Sonnet 4.6
Add this trailer to all affected commits during the rebase/squash step (see the Process section
below).
Should fix
4. Redux isLoading is not scoped to a workspace
store/Kubeconfig/reducer.ts — a single isLoading: boolean covers all workspaces. If a user
has two workspace detail pages open in separate tabs, clicking refresh on one will show the
spinner on the other. Use local component state instead — this is a short-lived UI toggle with
no cross-component consumers:
// In RefreshKubeconfig, replace isRefreshing prop + Redux selector with:
private state = { isRefreshing: false };
private async handleRefreshKubeconfig(): Promise<void> {
this.setState({ isRefreshing: true });
try {
// ...
} finally {
this.setState({ isRefreshing: false });
}
}This also removes the need for kubeconfigRefreshRequestAction / kubeconfigRefreshSuccessAction
and the selectIsRefreshingKubeconfig selector. The Redux slice can keep only the error state
if you want global error surfacing — or remove it entirely if errors are only shown as toasts.
5. Error is handled twice
store/Kubeconfig/actions.ts — the thunk dispatches kubeconfigRefreshErrorAction(errorMessage)
and then re-throws e. The component's catch block then shows the toast. The Redux error
state is never read by any component (no usage in the diff). Either:
- Remove the Redux error tracking entirely and let the re-throw reach the component catch, or
- Keep Redux error tracking and remove the component-level catch (but then provide a UI consumer
forselectKubeconfigRefreshError).
As written, the Redux error field is set but never displayed.
Minor
6. aria-live missing on spinner
RefreshKubeconfig/index.tsx — the <Spinner> has aria-label="Refreshing kubeconfig" but
the appearance / disappearance is not announced to screen readers dynamically. Wrap with an
aria-live region:
<span aria-live="polite" aria-atomic="true">
{isRefreshing && (
<Spinner size="md" className={styles.spinner} aria-label="Refreshing kubeconfig" />
)}
</span>7. Consider making podmanLogin failure non-blocking
store/Kubeconfig/actions.ts — if podmanLogin fails (e.g., no external registries configured),
the entire operation is marked as failed, even though injectKubeConfig already succeeded.
Consider catching podmanLogin errors separately and surfacing them as a warning instead.
8. Test uses unsafe ref.current as unknown as {...} cast
WorkspaceDetails/__tests__/index.spec.tsx:678 — the cast bypasses type safety and will silently
break if handleOnSave is renamed or its signature changes. Trigger the save through the UI
(screen.getByRole('button', { name: 'Update workspace' }) + userEvent.click) to make the
test resilient to refactoring.
Process
Rebase onto main
The branch needs a rebase onto the current main. If you are using Claude to generate code,
tell it to accept (not overwrite) .claude/rules and .claude/skills during conflict resolution
— those files contain project conventions and should always be taken from main.
Squash commits
The branch has 12 commits. Please squash them down to 3–4 logical commits, for example:
feat: add Advanced tab to Workspace Details page
feat(store): add Kubeconfig Redux slice with refresh action
test: add unit tests for Kubeconfig store and RefreshKubeconfig component
Use git rebase -i origin/main and mark intermediate commits as fixup or squash.
Sign-off all commits
All commits must carry a Signed-off-by trailer (DCO). Use -s when committing:
git commit -s -m "feat: add Advanced tab to Workspace Details page"To add sign-off to existing commits during the interactive rebase, run:
git rebase --signoff origin/main
batleforc
force-pushed
the
kube-token-sync-v2
branch
from
01bd52b to
362ca36
Compare
refresh kubeconfig button Assisted-by: Claude Sonnet 4.6 feat: temporary change feat: again feat: Validate with Che that the dashboard-frontend refresh work feat: disable on stopped ws feat: remove the defer patern and add the podmanLogin step has adviced by @olexii4 feat: add test suite for refreshKubeconfigWorkspace feat: Heavy lifting for the advanced tab Assisted-by: Claude Sonnet 4.6 feat: add new test, cover the tab link for workspace details, default fallback Assisted-by: Claude Sonnet 4.6 Signed-off-by: Max Batleforc <maxleriche.60@gmail.com>
Assisted-by: Claude Sonnet 4.6 Signed-off-by: Max Batleforc <maxleriche.60@gmail.com>
Assisted-by: Claude Sonnet 4.6 Signed-off-by: Max Batleforc <maxleriche.60@gmail.com>
batleforc
force-pushed
the
kube-token-sync-v2
branch
from
362ca36 to
00f96a8
Compare
|
@olexii4 I applied the fix and for the refreshKubeconfig i gave him the full direct workspace object. The PR/dash-licences CI broke for those: Is there any way to fix it ? |
@batleforc I have fixed it. |
Thanks, the MR is good for me, if there is any other feedback or thing to add related to the addition i can do so. |
|
@batleforc: 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. |
|
/che-ai-assistant check-pr-test-failures Review is complete. Please check the review comments below. |
|
PR #1492 -- eclipse-che/che-dashboard -- Kube token sync v2
FAILED docker-build (GHA)The docker-build workflow failed during the Docker registry login step. The docker/login-action@v3 attempted to authenticate with quay.io but encountered an error: "Username and password required". The workflow is missing the required credentials (QUAY_USERNAME and QUAY_PASSWORD secrets) needed to push the container image to the quay.io/eclipse registry. Log: docker-build FAILED ci/prow/v19-dashboard-happy-path (Prow)Prow logs could not be fetched automatically. The build logs are dynamically loaded and require manual inspection to determine the root cause of the failure. This job runs E2E happy path tests for the dashboard on OpenShift v19. Log: ci/prow/v19-dashboard-happy-path FAILED ci/prow/v19-e2e-puppeteer (Prow)Prow logs could not be fetched automatically. The build logs are dynamically loaded and require manual inspection to determine the root cause of the failure. This job runs E2E tests using Puppeteer for the dashboard on OpenShift v19. Log: ci/prow/v19-e2e-puppeteer FAILED ci/prow/v19-images (Prow)The Prow job failed due to a "Pod scheduling timeout" error. This indicates that the job pod could not be scheduled on any available node within the allocated time window, which is typically an infrastructure issue rather than a test failure. The failure is likely due to insufficient cluster resources, node unavailability, or resource quota constraints in the OpenShift CI environment. Log: ci/prow/v19-images |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: batleforc, 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 |
7 participants
What does this PR do?
Add an item to the WS dropdown that allow re-injecting the user's kube token inside the WS
Screenshot/screencast of this PR
What issues does this PR fix or reference?
eclipse-che/che#23545
Is it tested? How?
Release Notes
Docs PR
Do i need to do a docs PR ?