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

feat(ec2): dry run connection script to surface errors earlier. #6037

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

Conversation

Hweinstock
Copy link
Contributor

@Hweinstock Hweinstock commented Nov 15, 2024

Problem

Follow up to #6018 (comment)

Solution

  • Run ssh within the same env as it will be run on real connection.
  • Log any resulting errors, and inform user where the process failed.
  • Also part of this PR is moving some functions to remoteSession.ts that are general enough to be there.

Error msg:
Screenshot 2024-11-15 at 5 53 51 PM


License: I confirm that my contribution is made under the terms of the Apache 2.0 license.

Copy link

  • This pull request modifies code in src/* but no tests were added/updated.
    • Confirm whether tests should be added or ensure the PR description explains why tests are not required.
  • This pull request implements a feat or fix, so it must include a changelog entry (unless the fix is for an unreleased feature). Review the changelog guidelines.
    • Note: beta or "experiment" features that have active users should announce fixes in the changelog.
    • If this is not a feature or fix, use an appropriate type from the title guidelines. For example, telemetry-only changes should use the telemetry type.

): Promise<void> {
try {
const env = { SESSION_ID: session.SessionId, STREAM_URL: session.StreamUrl, TOKEN: session.TokenValue }
await new ProcessClass(sshPath, ['-T', `${user}@${hostname}`, 'echo connected && exit']).run({
Copy link
Contributor

Choose a reason for hiding this comment

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

not a blocker: for connecting to a Windows machine would we need a different test command? or does this command work on powershell?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since we directly invoke the sshPath I believe this should still work. However, once I add some tests for this function I will hopefully have a better idea.

Copy link
Contributor

@justinmk3 justinmk3 left a comment

Choose a reason for hiding this comment

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

Awesome

): Promise<void> {
try {
const env = { SESSION_ID: session.SessionId, STREAM_URL: session.StreamUrl, TOKEN: session.TokenValue }
await new ProcessClass(sshPath, ['-T', `${user}@${hostname}`, 'echo connected && exit']).run({
Copy link
Contributor

@justinmk3 justinmk3 Nov 16, 2024

Choose a reason for hiding this comment

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

can we capture the output of this process invocation and include it in the logs?

we may even want to use a similar approach as collectSamErrors (which should probably be lifted into errors.ts, and generalized a bit) to collect errors, and include them in the user-facing message.

export function collectSamErrors(samOutput: string): string[] {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe the output is already in the logs because we bind a logger to the process:

const SessionProcess = createBoundProcess(envProvider).extend({
onStdout: logger,
onStderr: logger,
rejectOnErrorCode: true,
})

Do you think its worth filtering the logs? I agree that collectSamErrors method could definitely be generalized and potentially applied here as a filter, but unsure what to filter for.

@Hweinstock Hweinstock changed the title feat(ec2): dry run connection script to surface errors earlier. (WIP) feat(ec2): dry run connection script to surface errors earlier. Nov 18, 2024
@Hweinstock Hweinstock marked this pull request as ready for review November 19, 2024 13:55
@Hweinstock Hweinstock requested a review from a team as a code owner November 19, 2024 13:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants