Skip to content

Conversation

@dkwon17
Copy link
Collaborator

@dkwon17 dkwon17 commented Sep 10, 2024

What does this PR do?

Adds documentation about ssh passphrase when automounting bashrc with a configmap

What issues does this PR fix or reference?

#1317

Is it tested? How?

PR Checklist

  • E2E tests pass (when PR is ready, comment /test v8-devworkspace-operator-e2e, v8-che-happy-path to trigger)
    • v8-devworkspace-operator-e2e: DevWorkspace e2e test
    • v8-che-happy-path: Happy path for verification integration with Che

@AObuchow
Copy link
Collaborator

@dkwon17 thanks for the PR :) will review shortly

@AObuchow
Copy link
Collaborator

CI is failing due to #1314

Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Thanks for following up #1307 with the necessary documentation @dkwon17 😁 This looks great to me. I left some minor comments for your consideration.

--from-literal=passphrase="$PASSPHRASE"
----
+
Note: If a passphrase is provided, the DevWorkspace operator adds a postStart event that starts the SSH agent and adds the passphrase.
Copy link
Collaborator

Choose a reason for hiding this comment

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

For consistency with the rest of the document: "DevWorkspace operator" -> "DevWorkspace Operator"

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, maybe we should bold "Note:"?
i.e. Note: -> *Note:*

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed, I've also updated all Note: do be bold

----
+
Note: If a passphrase is provided, the DevWorkspace operator adds a postStart event that starts the SSH agent and adds the passphrase.
The DevWorkspace operator also modifies the bashrc file to configure the `SSH_AGENT_PID` and `SSH_AUTH_SOCK` environment variables.
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • "operator" should also be capitalized here for consistency
  • Maybe we should replace all mentions of "bashrc" with `~/.bashrc`?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed both

Note: If a passphrase is provided, the DevWorkspace operator adds a postStart event that starts the SSH agent and adds the passphrase.
The DevWorkspace operator also modifies the bashrc file to configure the `SSH_AGENT_PID` and `SSH_AUTH_SOCK` environment variables.
If you are automatically mounting your own bashrc file with a ConfigMap (see link:additional-configuration.adoc#automatically-mounting-volumes-configmaps-and-secrets[Automatically mounting volumes, configmaps, and secrets])
you must add the following in your bashrc file:
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would suggest changing in your .bashrc file: to to your ~/.bashrc:. Though this is just a suggestion -- the way you currently have it makes sense 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

+
[source,bash]
----
[ -f /home/user/ssh-environment ] && source /home/user/ssh-environment
Copy link
Collaborator

Choose a reason for hiding this comment

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

In the implementation of the postStart event, $HOME is used instead of /home/user/ as the workspace may be using a different image than the UDI, which may have a different value for $HOME.

So maybe this should instead be:
[ -f $HOME/ssh-environment ] && source $HOME/ssh-environment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@AObuchow
Copy link
Collaborator

Now that #1315 is merged, you should be good to rebase your PR onto main and the CI checks should pass @dkwon17 :)

Signed-off-by: dkwon17 <dakwon@redhat.com>
Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Last comment, then this will be good to merge (just remember to squash your changes). Feel free to just amend your last commit & squash things so that we can merge ASAP :)

----
+
*Note:* If a passphrase is provided, the DevWorkspace Operator adds a postStart event that starts the SSH agent and adds the passphrase.
The DevWorkspace operator also modifies the `~/.bashrc` to configure the `SSH_AGENT_PID` and `SSH_AUTH_SOCK` environment variables.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry for the last nitpick: "operator" should also be capitalized here, i.e. "The DevWorkspace Operator"

@AObuchow
Copy link
Collaborator

@dkwon17 I quickly opened #1317 so we can track this in the DWO 0.31.0 milestones 😎

Signed-off-by: dkwon17 <dakwon@redhat.com>
Signed-off-by: dkwon17 <dakwon@redhat.com>
Copy link
Collaborator

@AObuchow AObuchow left a comment

Choose a reason for hiding this comment

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

Looks good to me, great work David thanks for getting this done :)
Please squash your last two "update" commits into the first commit, and merge this when you have a chance 👍

@openshift-ci
Copy link

openshift-ci bot commented Sep 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: AObuchow, dkwon17

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

@dkwon17 dkwon17 merged commit 419adb5 into devfile:main Sep 11, 2024
AObuchow pushed a commit to mancubus77/devworkspace-operator that referenced this pull request Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants