Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix missing Terminal Shortcuts for "shoot" and "cp" scopes. #2339

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Lappihuan
Copy link

What this PR does / why we need it:
This fixes a bug that broke all "shoot" and "cp" scope while refactoring the state management:
4b4c510#diff-abeb7e9d3448e309c3fc4a7cc959611852d5242b0fcf58540b063f494b131b04L41

The prop shootItem used to be passed to GTerminalShortcuts component, this was removed but the component still relies on the object to be present for it to include the terminal shortcuts as seen here:

if (this.shootItem) {
targetsFilter.push(TargetEnum.SHOOT, TargetEnum.CONTROL_PLANE)
}

But this.shootItem was never set anymore in any parent component as prop:
https://github.com/search?q=repo%3Agardener%2Fdashboard+%22%3Cg-terminal-shortcuts%22&type=code

this PR get the shootItem via the useShootItem composable the same way as in the GTerminalShortcut.vue:
4b4c510#diff-3925a06cd814d08ed6d969e9c3553128a5b66d2b5415c735374bd5b1fe2315efR132-R142

With this change all three scopes "garden", "cp" and "shoot" shortcuts are shown again correctly

Which issue(s) this PR fixes:

Special notes for your reviewer:

Release note:

fixes missing terminal shortcuts for "shoot" and "cp" scopes

@CLAassistant
Copy link

CLAassistant commented Feb 28, 2025

CLA assistant check
All committers have signed the CLA.

@gardener-robot
Copy link

@Lappihuan Thank you for your contribution.

@gardener-robot-ci-2
Copy link
Contributor

Thank you @Lappihuan for your contribution. Before I can start building your PR, a member of the organization must set the required label(s) {'reviewed/ok-to-test'}. Once started, you can check the build status in the PR checks section below.

@gardener-robot gardener-robot added the size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) label Feb 28, 2025
…andled anywhere from any parent component

Signed-off-by: Oriano de-Stefani <[email protected]>
… the "GARDEN CLUSTER" button and going to terminal shortcuts from there

Signed-off-by: Oriano de-Stefani <[email protected]>
@gardener-robot
Copy link

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

Copy link
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.

Thank you for providing a fix. I just have minor suggestions

@@ -129,9 +129,9 @@ export default {
],
setup () {
const {
shootItem,
shootItem = null,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
shootItem = null,
shootItem,

Copy link
Author

@Lappihuan Lappihuan Mar 3, 2025

Choose a reason for hiding this comment

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

this would break the context when coming from the button "GARDEN CLUSTER", since then useShootItem seems to return null.

GTerminalShortcuts.vue:135 Uncaught (in promise) TypeError: Cannot destructure property 'shootItem' of 'useShootItem(...)' as it is null.

this just tries to handle this gracefully so we still end up with shootItem as null.

there might be a better way to solve it in useShootItem to still return a result object instead of just null

Copy link
Author

Choose a reason for hiding this comment

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

this seems to come from this default value here:

export function useShootItem () {
return inject('shoot-item', null)
}

isShootStatusHibernated,
} = useShootItem()
} = useShootItem() || {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
} = useShootItem() || {}
} = useShootItem()

Copy link
Author

Choose a reason for hiding this comment

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

same as above

Comment on lines +135 to +136
shootItem = null,
} = useShootItem() || {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
shootItem = null,
} = useShootItem() || {}
shootItem,
} = useShootItem()

Copy link
Author

Choose a reason for hiding this comment

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

same as above

@Lappihuan Lappihuan requested a review from petersutter March 3, 2025 15:27
@gardener-robot
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants