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

Bug 2104859: Add "Copy SSH command" to VM actions #822

Conversation

hstastna
Copy link
Member

@hstastna hstastna commented Aug 8, 2022

📝 Description

Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2104859
https://bugzilla.redhat.com/show_bug.cgi?id=2117803

Add "Copy SSH command" option to kebab menu for the actions in the VM list, also to Actions button in the VM details view/tabs, to copy the command fast and comfortably.

Make editing SSH access field available in the VM Details tab, even when the VM is stopped.

Add missing popovers for "SSH over NodePort" and "SSH using virtctl" in the VM Details tab according to the design doc.

Design doc:
https://docs.google.com/document/d/1FyTSLtTBgl4wJn4BJO-lTeG7rGA9cq1eWlH-a5c0jHI/edit#heading=h.r9srzahlc6ei

🎥 Demo

Before:
The Actions menu before:
copy_before
After:
"Copy SSH command" in the VM Overview tab:
c1
c2
"Copy SSH command" in the VM list view:
c3
Popovers in the VM Details tab:
pop1
pop2

TODO:

  • use virtctl command for "Copy SSH command"
  • display the PF copy icon next to the text "Copy SSH command"
  • enable "Copy SSH command" for the VM that is stopped
  • fix failing test
  • add a description for "Copy SSH command": "SSH using virtctl"

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 8, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

@openshift-ci openshift-ci bot requested a review from gouyang August 8, 2022 16:58
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 8, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

@hstastna
Copy link
Member Author

hstastna commented Aug 8, 2022

@glekner @avivtur @metalice @pcbailey @yzamir @vojtechszocs please review
Can you help me, folks, to find a reason of not displaying the copy icon as expected? Expected:
copy
And the related line of the code here.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 8, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

Copy link
Member

@avivtur avivtur left a comment

Choose a reason for hiding this comment

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

Hi @hstastna the PR looks great 😄
in order for you to show the icon you need to go to here and add the line:
icon={action?.icon}
but the PF dropdown will show the icon before the rendered child as written in their docs, I think it's ok since it's coming from PF and I wouldn't try to change that (and not sure if it is possible 😆 )
Can I also ask of you please to move this directory to here as well in this PR? it makes more sense to me to have the actions compnent under the actions directory, I know you didn't made it like this, but it will be nice that it will be changed :)
Also make sure to apply needed changes to this unit-test to make our CI happy again :)

@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch from 918b655 to 59bb1b3 Compare August 9, 2022 14:28
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

2 similar comments
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 9, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch 2 times, most recently from 3c7137a to 958ffa8 Compare August 9, 2022 14:55
@hstastna
Copy link
Member Author

hstastna commented Aug 9, 2022

@avivtur Here's one another thing that bothers me: in the design doc for this, there's "Copy SSH command" using virtctl for the drop down item that was already implemented in the VM Overview tab. But when I check it, it does not seem to use virtctl. I'd expect to see some command like virtctl -n ... when I copy that command. Yifat thinks that the doc is correct. What am I missing? Did we miss virtctl when implementing or was there any design change I and Yifat are not aware of? Maybe @upalatucci knows more? Thanks!

@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch 2 times, most recently from 11351f1 to d787d45 Compare August 9, 2022 16:01
@hstastna hstastna requested a review from avivtur August 9, 2022 16:21
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 10, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

@hstastna
Copy link
Member Author

/retest

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 10, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch 4 times, most recently from 287c27d to e8ed9f0 Compare August 12, 2022 12:54
@hstastna hstastna changed the title Bug 2104859: Add "Copy SSH command" to VM actions [WIP] Bug 2104859: Add "Copy SSH command" to VM actions Aug 12, 2022
@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch from e8ed9f0 to 3aa48d1 Compare August 12, 2022 16:11
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

@hstastna: This pull request references Bugzilla bug 2104859, which is valid.

3 validation(s) were run on this bug
  • bug is open, matching expected state (open)
  • bug target release (4.12.0) matches configured target release for branch (4.12.0)
  • bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @gouyang

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

@vojtechszocs
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added lgtm Passed code review, ready for merge approved This issue is something we want to fix labels Aug 25, 2022
@vojtechszocs
Copy link
Contributor

/lgtm cancel

@openshift-ci openshift-ci bot removed lgtm Passed code review, ready for merge approved This issue is something we want to fix labels Aug 25, 2022
@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch 2 times, most recently from 0e04bb3 to cc33ddd Compare August 25, 2022 20:13
@vojtechszocs
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added lgtm Passed code review, ready for merge approved This issue is something we want to fix labels Aug 25, 2022
@openshift-bot
Copy link

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch from cc33ddd to a826ada Compare August 25, 2022 20:33
@openshift-ci openshift-ci bot removed the lgtm Passed code review, ready for merge label Aug 25, 2022
@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch from a826ada to 63f1cc4 Compare August 25, 2022 20:36
@vojtechszocs
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label Aug 25, 2022
Add "Copy SSH command" option to kebab menu for the actions in the VM
list, also to Actions button in the VM details view/tabs (SSH using
virtctl).

Make editing SSH access field available in the VM Details tab,
even when the VM is stopped.

Add missing popovers for "SSH over NodePort" and "SSH using virtctl"
in the VM Details tab according to the design doc.

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2104859
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2117803
@hstastna hstastna force-pushed the RFE_Add_Copy_SSH_command_VM_actions branch from 63f1cc4 to 90fadef Compare August 25, 2022 20:46
@openshift-ci openshift-ci bot removed the lgtm Passed code review, ready for merge label Aug 25, 2022
@vojtechszocs
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Passed code review, ready for merge label Aug 25, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hstastna, vojtechszocs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit fb88028 into kubevirt-ui:main Aug 25, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 25, 2022

@hstastna: All pull requests linked via external trackers have merged:

Bugzilla bug 2104859 has been moved to the MODIFIED state.

In response to this:

Bug 2104859: Add "Copy SSH command" to VM actions

Instructions 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/test-infra repository.

@hstastna
Copy link
Member Author

/cherry-pick release-4.11

@openshift-cherrypick-robot
Copy link
Collaborator

@hstastna: #822 failed to apply on top of branch "release-4.11":

Applying: Bug 2104859: Add "Copy SSH command" to VM actions
Using index info to reconstruct a base tree...
M	locales/en/plugin__kubevirt-plugin.json
A	src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceNotConfigured.tsx
A	src/utils/components/SSHAccess/components/ConsoleOverVirtctl.tsx
A	src/utils/components/SSHAccess/components/SSHCommand.tsx
A	src/utils/components/SSHAccess/useSSHCommand.ts
M	src/views/virtualmachines/actions/VirtualMachineActionFactory.tsx
M	src/views/virtualmachines/actions/hooks/useVirtualMachineActionsProvider.ts
M	src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabDetails/components/VirtualMachinesOverviewTabDetailsTitle.tsx
M	src/views/virtualmachines/list/components/VirtualMachineActions/VirtualMachineActions.tsx
Falling back to patching base and 3-way merge...
Auto-merging src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabDetails/components/VirtualMachinesOverviewTabDetailsTitle.tsx
CONFLICT (content): Merge conflict in src/views/virtualmachines/details/tabs/overview/components/VirtualMachinesOverviewTabDetails/components/VirtualMachinesOverviewTabDetailsTitle.tsx
Auto-merging src/views/virtualmachines/actions/hooks/useVirtualMachineActionsProvider.ts
Auto-merging src/views/virtualmachines/actions/components/VirtualMachineActions/VirtualMachineActions.tsx
Auto-merging src/views/virtualmachines/actions/VirtualMachineActionFactory.tsx
Auto-merging src/utils/components/UserCredentials/useSSHCommand.ts
CONFLICT (modify/delete): src/utils/components/SSHAccess/components/SSHCommand.tsx deleted in HEAD and modified in Bug 2104859: Add "Copy SSH command" to VM actions. Version Bug 2104859: Add "Copy SSH command" to VM actions of src/utils/components/SSHAccess/components/SSHCommand.tsx left in tree.
CONFLICT (modify/delete): src/utils/components/SSHAccess/components/ConsoleOverVirtctl.tsx deleted in HEAD and modified in Bug 2104859: Add "Copy SSH command" to VM actions. Version Bug 2104859: Add "Copy SSH command" to VM actions of src/utils/components/SSHAccess/components/ConsoleOverVirtctl.tsx left in tree.
CONFLICT (modify/delete): src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceNotConfigured.tsx deleted in HEAD and modified in Bug 2104859: Add "Copy SSH command" to VM actions. Version Bug 2104859: Add "Copy SSH command" to VM actions of src/utils/components/Consoles/components/DesktopViewer/Components/RDPServiceNotConfigured.tsx left in tree.
Auto-merging locales/en/plugin__kubevirt-plugin.json
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Bug 2104859: Add "Copy SSH command" to VM actions
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick release-4.11

Instructions 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/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This issue is something we want to fix bugzilla/severity-medium bugzilla/valid-bug lgtm Passed code review, ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants