Skip to content

Fix Create Terminal Dialog Issues#2373

Merged
grolu merged 13 commits intomasterfrom
bug/fix_terminal_issues
Apr 24, 2025
Merged

Fix Create Terminal Dialog Issues#2373
grolu merged 13 commits intomasterfrom
bug/fix_terminal_issues

Conversation

@grolu
Copy link
Copy Markdown
Member

@grolu grolu commented Mar 26, 2025

What this PR does / why we need it:

Which issue(s) this PR fixes:
Fixes #2170

Special notes for your reviewer:

Release note:

Fixed several issues with Create Terminal Dialog:
- Disable terminal scheduling on non-managed seeds
- Added error handling in case cluster nodes cannot be retrieved
- Moved alerts from scrollable container to fixed position to make them always visible

grolu added 4 commits March 26, 2025 22:36
@grolu
Do not fail terminal config call if shoot API server is unreachable
05831b8 
Handle case when no shoot nodes are available (empty selection)
Disable create button in this case
@grolu
Moved alerts from subcomponents to GCreateTerminalSessionDialog to mo…
23ee59f 
…ve them out of scrollable container

This ensures that they are always visible
@grolu
Apply suggestions from @Lappihuan PR #2358
20542bf
@grolu
Proposal for disabling terminal scheduling on non-managed seeds
51f0f1f
@gardener-robot gardener-robot added needs/review Needs review size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Mar 26, 2025
@ghost ghost added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 26, 2025
@grolu grolu changed the title Fix terminal issues Fix Create Terminal Dialog Issues Mar 26, 2025
@Lappihuan Lappihuan mentioned this pull request Mar 27, 2025
Closed
@grolu
Fixed create disabled for some scenarios
abf3540 
Added alert with error message in case no nodes are available to schedule terminal
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Mar 28, 2025
@Lappihuan
Copy link
Copy Markdown
Contributor

Lappihuan commented Mar 28, 2025 •
edited
Loading

@grolu i tested this PR and the terminal dialog behaves how it should.
however shortcuts don't behave correctly.

Shortcuts that target cp and shoot both error with terminal cannot be hosted on non-managed seed.
shoot at least should work.

I would make sense to filter out the Shortcuts that target cp in that scenario.

Shortcuts targeting garden is created but i actually can't connect to the virtual garden, but that seems to be unrelated to this issue.
Or am i missunderstanding the garden target?

@gardener-robot
Copy link
Copy Markdown

gardener-robot commented Mar 29, 2025

@holgerkoser You have pull request review open invite, please check

@grolu
Disable terminal shortcuts for target control plane
6c2ac4c 
Disable terminal shortcuts for target shoot for operators
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 2, 2025
@grolu
- Removed loading placeholder
10e493c 
- Fixed flaky initial default target
- Fixed terminal component initialization issue during navigation (loading state set too early, causing router-view to be returned before the navigation took place)
@gardener-robot gardener-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs/second-opinion Needs second review by someone else and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 2, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 2, 2025
@ghost ghost removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 2, 2025
@grolu
Copy link
Copy Markdown
Member Author

grolu commented Apr 2, 2025

Hi @Lappihuan
I fixed some last things:

  • Fixed a navigation issue when navigating back from terminal
  • Fixed initial CreateTerminalSession dialog rendering
  • Improved tooltips

Shortcuts that target cp and shoot both error with terminal cannot be hosted on non-managed seed.
shoot at least should work.

  • Control Plane terminal shortcut targets are now disabled in case seed is not managed
  • For Operators, terminal shortcuts with target shoot are also hosted on the seed, therefore they are now also disabled in case the seed is not managed. Of course, they could be scheduled on the shoot. But enabling this for operators would be cumbersome - we would need to show warnings etc. like we do on the create terminal session dialog.

Shortcuts targeting garden is created but i actually can't connect to the virtual garden, but that seems to be unrelated to this issue.

Probably unrelated, garden terminals for operators are scheduled on a dedicated cluster. Maybe something is wrong with your config. There is also a related issue: #1789

@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 2, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 11, 2025
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 14, 2025
@ghost ghost removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 14, 2025
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 16, 2025
petersutter
petersutter reviewed Apr 17, 2025
View reviewed changes
Comment thread frontend/src/components/dialogs/GCreateTerminalSessionDialog.vue Outdated
petersutter
petersutter reviewed Apr 17, 2025
View reviewed changes
Comment thread frontend/src/components/GTerminalShortcut.vue Outdated
petersutter
petersutter reviewed Apr 17, 2025
View reviewed changes
Comment thread frontend/src/components/dialogs/GCreateTerminalSessionDialog.vue
petersutter
petersutter reviewed Apr 17, 2025
View reviewed changes
Comment thread backend/lib/services/shoots.js
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Apr 22, 2025
petersutter
petersutter approved these changes Apr 24, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@petersutter petersutter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review needs/second-opinion Needs second review by someone else labels Apr 24, 2025
@ghost ghost added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 24, 2025
@gardener-robot gardener-robot added needs/second-opinion Needs second review by someone else and removed reviewed/lgtm Has approval for merging labels Apr 24, 2025
@ghost ghost removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Apr 24, 2025
@grolu grolu merged commit 2501cc2 into master Apr 24, 2025
11 checks passed
@grolu grolu deleted the bug/fix_terminal_issues branch April 24, 2025 09:24
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Apr 24, 2025
@grolu grolu added the area/ipcei IPCEI (Important Project of Common European Interest) label May 27, 2025
@grolu grolu mentioned this pull request May 27, 2025
Open
@grolu grolu mentioned this pull request Oct 15, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@petersutter petersutter petersutter approved these changes

@holgerkoser holgerkoser Awaiting requested review from holgerkoser holgerkoser is a code owner

Assignees

No one assigned

Labels

area/ipcei IPCEI (Important Project of Common European Interest) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/second-opinion Needs second review by someone else size/L Denotes a PR that changes 100-499 lines, ignoring generated files. status/closed Issue is closed (either delivered or triaged)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Disable terminal scheduling on non-managed seeds in the dashboard

6 participants

@grolu @Lappihuan @gardener-robot @petersutter @gardener-robot-ci-1 @gardener-robot-ci-2