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

squash ssh agent warnings if -Quiet #484

Merged
merged 4 commits into from
Sep 15, 2017
Merged

squash ssh agent warnings if -Quiet #484

merged 4 commits into from
Sep 15, 2017

Conversation

refack
Copy link
Contributor

@refack refack commented Sep 14, 2017

Maps better to stdout/stderr, so cases where PS is run as CLI tool
Refs: nodejs/node-gyp#1195 (comment)

@dahlbyk
Copy link
Owner

dahlbyk commented Sep 14, 2017

Hrm... most of these warnings aren't meant to be fatal (e.g. the posh-git default profile tries to start ssh-agent, but it's not a huge deal if it can't).

I do feel like -NoProfile, as you've proposed in nodejs/node-gyp#1292, is the more correct solution to addressing these warnings. This module really isn't meant to be loaded non-interactively.

@refack
Copy link
Contributor Author

refack commented Sep 14, 2017

I do feel like -NoProfile, as you've proposed in nodejs/node-gyp#1292, is the more correct solution to addressing these warnings. This module really isn't meant to be loaded non-interactively.

Gray area, agreed.
Only issue is that node-gyp has a long-ish release cycle (first need to land PR, then cut a new version, then get npm to adopt it, then get people to update npm), and I assume posh-git is more agile.

P.S. Looks like a nice tool, I'll give it a try.

@dahlbyk
Copy link
Owner

dahlbyk commented Sep 14, 2017

Only issue is that node-gyp has a long-ish release cycle (first need to land PR, then cut a new version, then get npm to adopt it, then get people to update npm), and I assume posh-git is more agile.

It is, but we also don't have an auto-update mechanism, so who knows how long until folks think to upgade?

That said, I'm quite confident this is only going to be hit by users that don't care about ssh-agent who are hitting Start-SshAgent -Quiet in the default profile. What if we only changed that function to suppress the two warnings altogether if -Quiet?

@refack
Copy link
Contributor Author

refack commented Sep 14, 2017

What if we only changed that function to suppress the two warnings altogether if -Quiet?

Sure.
(I did the editing in the GitHub GUI, let me clone and force push)

@refack refack changed the title use Write-Error instead of Write-Warning squash ssh agent warnings if -Quiet Sep 14, 2017
@refack
Copy link
Contributor Author

refack commented Sep 14, 2017

Done

@dahlbyk
Copy link
Owner

dahlbyk commented Sep 15, 2017

Close, but you updated Add-SshAgent, not Start-SshAgent!

@dahlbyk
Copy link
Owner

dahlbyk commented Sep 15, 2017

Actually, Start-SshAgent calls Add-SshAgent so we should probably keep the $Quiet guard in both. Sorry for churn on this. 😞

@dahlbyk
Copy link
Owner

dahlbyk commented Sep 15, 2017

@refack I've wasted enough of your time... made the rest of the changes, if you care to review?

@dahlbyk dahlbyk merged commit fd7d398 into dahlbyk:master Sep 15, 2017
@refack
Copy link
Contributor Author

refack commented Sep 15, 2017

@refack I've wasted enough of your time... made the rest of the changes, if you care to review?

No time wasted... Cooperation is always a win 🏆

@refack refack deleted the patch-1 branch September 15, 2017 20:38
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