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

Update SSH Agent FAQ for Windows #96

Merged
merged 1 commit into from
Mar 7, 2022
Merged

Update SSH Agent FAQ for Windows #96

merged 1 commit into from
Mar 7, 2022

Conversation

Okeanos
Copy link
Contributor

@Okeanos Okeanos commented Sep 2, 2021

Previously Windows' own OpenSSH option was not mentioned, even though KeePassXC supports it.
Additionally, to better differentiate between the two major platforms (Unix vs. Window) and their differences they now each have a separate section.

Some additional comments:

I removed the line following line from the Why is Pageant refusing my keys?-question:

There doesn't seem to be any alternative to Pageant for Windows that supports both of them.

Because from a cursorily glance at the Win32 OpenSSH issues there may be options to correctly provide and configure SSH_ASKPASS when using Windows' own OpenSSH binaries. I haven't verified that.

There is "a lot" of duplication between the two sections – yeah, that's true. It might be useful to split that into a generalised third section. Thoughts?

It might be worth a shot to add a question/answer pair outlining the "use KeePassXC together with Git for Windows" workaround/solution I referred to in keepassxc#4681.

Assuming any changes have to be made to get this merged … I assume you want all of this in a single commit?

As an aside, unrelated to this pull request: My IDE and I noticed some trailing whitespaces throughout the file and generally inconsistent formatting (e.g. <br> vs <br />) and some technical problems (such as missing references e.g. to #faq-string-fields or wrongly nested HTML elements <ul> inside of <ul> instead of <li>). Would you be interested in a separate pull request that fixes these things?

@phoerious phoerious requested a review from hifi September 3, 2021 11:33
@hifi
Copy link
Member

hifi commented Sep 4, 2021

@phoerious @droidmonkey should we just remove the outdated FAQ entries (not all) and point people to the docs? I don't think maintaining the FAQ as documentation like it was before is good use of anyone's time.

@Okeanos is the actual documentation at https://keepassxc.org/docs/KeePassXC_UserGuide.html#_ssh_agent more up to date and would you be interested in working on that? The doc sources are part of the main KeePassXC repository.

@phoerious
Copy link
Member

I would keep some FAQ, but if things can be explained better in the docs, I'm all for it.

@Okeanos
Copy link
Contributor Author

Okeanos commented Sep 4, 2021

The actual documentation does indeed look better and I'd be willing to see whether I can still improve it.

Reducing the FAQ and at the same time fixing some issues (like pointing out OpenSSH is supported on Windows) sounds like a good idea.

@droidmonkey
Copy link
Member

droidmonkey commented Sep 6, 2021

I dislike the FAQ for several reasons. That page is ridiculously large and impossible to navigate. The information tends to get stale quickly. Anything that is truly "documentation" should be put in our user guide instead. FAQ should be errata, not fundamental usage tips.

I dislike this change a lot because it makes the above points even worse.

@Okeanos
Copy link
Contributor Author

Okeanos commented Sep 15, 2021

I now reverted most of my original changes and instead opted to enrich the existing SSH Agent information with minimal additions concerning the Windows support.

If there are additional changes required before this can be merged I'd be happy to do that. I would like to see the FAQ changes merged because until a better version exists I would at least like to get rid of some inaccuracies for now.

Concerning the full rewrite/change of the FAQ and updates to the real user documentation: I would personally prefer to do that in a separate pull request; especially the former currently appears to require more discussion?

@droidmonkey
Copy link
Member

Documentation is edited on the keepassxc repo. https://github.com/keepassxreboot/keepassxc/tree/develop/docs

@Okeanos
Copy link
Contributor Author

Okeanos commented Sep 20, 2021

I updated the pull request and added a single point concerning interaction with Git For Windows – if there particular changes I have to make before this can be merged I'd be happy to do so.

droidmonkey pushed a commit to keepassxreboot/keepassxc that referenced this pull request Oct 2, 2021
* Update naming of `macOS` to use the official case instead of `MacOS`
* Add an in depth explanation for using Windows OpenSSH (see keepassxreboot/keepassxc-org#96 for details)
* Remove some trailing whitespace
@Okeanos
Copy link
Contributor Author

Okeanos commented Nov 21, 2021

@hifi I just noticed that this is still open and I am wondering whether there are any additional changes required or different plans altogether for the FAQ (that I might be able contribute to)? I'd appreciate some feedback, even if it's just closing/declining the pull request.

@Okeanos
Copy link
Contributor Author

Okeanos commented Mar 5, 2022

@droidmonkey @hifi I updated the pull request to resolve the conflicts with the current master. If there's anything else I need to change for this to get merged, please let me know.

@hifi
Copy link
Member

hifi commented Mar 5, 2022

Ah, sorry. I'll read these through and merge if nothing stands out. Thanks!

Copy link
Member

@hifi hifi left a comment

Choose a reason for hiding this comment

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

I read it through and noted some things that could be improved.

Sorry for forgetting this PR completely, it's not against the main repo so it doesn't pop up in my views that easily. 🙇

docs/index.html Outdated Show resolved Hide resolved
docs/index.html Outdated Show resolved Hide resolved
docs/index.html Show resolved Hide resolved
Previously Windows' own OpenSSH option was not mentioned, even though
KeePassXC supports it.
Different Windows options are explicitly mentioned some inaccurate
statements have been rephrased or enriched with some additional
context.
Add an FAQ entry for using KeePassXC SSH Agent integration with Git
for Windows and a caveat for the unsupported Git bundled OpenSSH.
@droidmonkey
Copy link
Member

@hifi good to go?

Copy link
Member

@hifi hifi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot for sticking through!

@hifi hifi merged commit 3bf44c5 into keepassxreboot:master Mar 7, 2022
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.

None yet

4 participants