-
Notifications
You must be signed in to change notification settings - Fork 52
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
Add passphrase input to the add SSH key dialog #1157
Conversation
f1637af
to
78c98d4
Compare
@vinokurig can you please take a look at why the build failed? |
f7704fa
to
74efd2c
Compare
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1157 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-1157", name: che-dashboard}]}}]" |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1157 +/- ##
==========================================
- Coverage 89.67% 89.56% -0.12%
==========================================
Files 415 416 +1
Lines 42461 42532 +71
Branches 2843 2846 +3
==========================================
+ Hits 38077 38092 +15
- Misses 4357 4412 +55
- Partials 27 28 +1 ☔ View full report in Codecov by Sentry. |
@vinokurig : could you. please, confirm, that it should be enough to install Eclipse Che Next with User Dashboard deployed from quay.io/eclipse/che-dashboard:pr-1157 to validate the PR? Asking, because the message in PR description mentioned devfile/devworkspace-operator#1291, so I am curious if it's needed to have specific devworkspace-operator as well. |
@dmytro-ndp For a full test flow in Che, I believe you'll need to use the DWO project clone container built from devfile/devworkspace-operator#1291 I've already built this image and pushed it here: To make DWO use it, you'll need to configure your DWOC as follows: kind: DevWorkspaceOperatorConfig
config:
workspace:
+ projectClone:
+ image: quay.io/aobuchow/project-clone:ihor-sshkey-pr Then create a new workspace for it to use the newly configured project clone container. This should work with the che-owned DWOC IIRC. |
@dmytro-ndp sorry, my above comment was wrong. What I described was testing the entire flow of the ssh passphrase feature. Based on @vinokurig's PR description:
I believe there should be a secret created in the user's namespace regarding the ssh key, and it should contain a passphrase field in it's data. Verifying that does not require a modified version of DWO or the project clone container. |
As @AObuchow mentioned in his comment above we do not need the modified DWO image here. In the scope of the pull request it would be enough to verify that the |
Thank you for the explanation, @AObuchow , @vinokurig ! I wanted to test whole cycle - starting from setting up git passphrase and ending by cloning the project inside the workspace editor using passphrase. I can do it in two steps:
|
@vinokurig: I faced error Test environment: OCP 4.16 cluster with Eclipse Che Next. using User Dashboard quay.io/eclipse/che-dashboard:pr-1157 , and DWOC quay.io/devfile/devworkspace-controller:sha-e176ec0 SSH key were created by the command: It would be great if you could take a look. |
@dmytro-ndp Thank you for reporting the error. I fixed the error in the latest commit. Also I changed the passphrase input type to password so the sensitive data is hidden: |
Docker image build succeeded: quay.io/eclipse/che-dashboard:pr-1157 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-1157", name: che-dashboard}]}}]" |
@vinokurig: The following test failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
@vinokurig : thanks for the improvement. I was successfully added ssh key with passphrase to the user preferences in dashboard quay.io/eclipse/che-dashboard@sha256:ac07ba98b10ee8b40eedfa4b8aae51f084a6396a627abdad774de6004b9a89cc = latest version of quay.io/eclipse/che-dashboard:pr-1157 and then created workspace from GitHub repo using this ssh key. Let.Dev.Spaces.keep.ssh.passphrase.for.git.webmWell done! At the same time, I faced an error
I had been asked to enter passphrase for key '/etc/ssh/dwo_ssh_key" when tried to push changes using Terminal: screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.08.06-14_39_32.webmIt didn't look like an expected behavior, IMHO. |
@vinokurig : one more notice: Uses Dashboard failed to add SSH key with passphrase containing Cyrillic symbols: SSH key were created using the command |
@dmytro-ndp Thank you for the deep testing and reporting the bugs. Despite that fact that the bugs are not related to the pull request, I think we should fix the bugs before merging the pull request. It is my bad that I did not try some git operations in the scope of the DWO pull request. |
@vinokurig , @AObuchow : here is the issue about pushing changes to the git repo: devfile/devworkspace-operator#1295 |
In this case I propose to postpone this feature and move it to 3.17 / 7.91 |
@dmytro-ndp The SSH flow works for both git UI and terminal commands, could you please validate the pull request again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vinokurig : fixed version Eclipse Che Next + quay.io/eclipse/che-dashboard@sha256:ac07ba98b10ee8b40eedfa4b8aae51f084a6396a627abdad774de6004b9a89cc worked as expected with ssh key using passphrase:
- push changes from the terminal in workspace
- push changes using GitHub Pull extension
Well done!
Screencast:
screencast-bpconcjcammlapcogcnnelfmaeghhagj-2024.09.13-21_39_37.webm
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Build 3.17 :: dashboard_3.x/543: Console, Changes, Git Data |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: akurinnoy, dmytro-ndp, ibuziuk, olexii4, vinokurig The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Build 3.17 :: sync-to-downstream_3.x/7703: Console, Changes, Git Data |
What does this PR do?
DO NOT MERGE
passphrase
value to the add SSH key dialog:passphrase
value to the ssh key secret.What issues does this PR fix or reference?
https://issues.redhat.com/browse/CRW-6614
Is it tested? How?
passphrase
input.git-ssh-key
secret data.See: the
passphrase
field is present in the data map and its value equals to the input.Release Notes
Docs PR