Skip to content
This repository has been archived by the owner on Jun 27, 2021. It is now read-only.

Search hostname:port/username #80

Merged
merged 1 commit into from
Apr 17, 2019

Conversation

kamidon
Copy link
Contributor

@kamidon kamidon commented Feb 14, 2019

According to README.md auth-source-pass supports entries with username
either prefixed to the hostname with an @ as separator or in a
subdirectory under the hostname. This was true when there was no port
or service included in the name, but not when there was one.

This commit adds a check for the missed case. I believe that it would
work just as well to replace the nil passed for the user in the call
two lines down, but I wasn't completely sure I wasn't missing
something for why nil was passed in that case so chose the
conservative option of adding the more specific check for the case
when both user and port are supplied.

@DamienCassou
Copy link
Owner

Thank you for your interest and your PR. It looks good. Could you please add a unit-test that fails without your fix and passes after?

Also, before I can merge your PR, I need you to sign the FSF copyright assignment form. Could you please send a mail to assign@gnu.org and ask for a form?

@kamidon
Copy link
Contributor Author

kamidon commented Feb 16, 2019

Thanks! I sent the email. I may not get to adding the test for a few days even though it should be straightforward. That's not from a lack of interest but just a lack of time in the near future. I'll get to it ASAP.

@kamidon
Copy link
Contributor Author

kamidon commented Feb 27, 2019

Added the requested test, which I verified fails without the change suggested in this PR and passes with it. I expect this test to get rewritten if I implement the changes we're discussing in PR 81 to return the list of all available matching entries and validate afterwards, but I think it is useful to merge this to capture the case that wasn't working first.

Unfortunately, I still need to complete the FSF paperwork, so this can't be merged quite yet.

@DamienCassou
Copy link
Owner

I think it is useful to merge this to capture the case that wasn't working first.

I agree. Thank you for your work.

Unfortunately, I still need to complete the FSF paperwork, so this can't be merged quite yet.

Please tell me when you send the form. I will merge at this point.

@kamidon
Copy link
Contributor Author

kamidon commented Mar 4, 2019

A quick update that I'm waiting on getting my Employer Disclaimer signed to complete the FSF copyright assignment process.

@DamienCassou
Copy link
Owner

Thanks for keeping me updated.

@DamienCassou
Copy link
Owner

Are you still waiting?

@kamidon
Copy link
Contributor Author

kamidon commented Apr 8, 2019

Yeah, I got an acknowledgement from the FSF that they are reviewing the documents an few weeks ago and haven't heard anything since. I'll check with them again.

@DamienCassou
Copy link
Owner

Yeah, I got an acknowledgement from the FSF

that's good enough for me. Would you mind updating your PRs please?

@kamidon
Copy link
Contributor Author

kamidon commented Apr 9, 2019

Sure, I'll probably have time to do that on the weekend.

@kamidon
Copy link
Contributor Author

kamidon commented Apr 15, 2019

I'm sorry I didn't get to this over the weekend because of other priorities. I still hope to get to it in the next few days.

According to README.md auth-source-pass supports entries with username
either prefixed to the hostname with an @ as separator or in a
subdirectory under the hostname.  This was true when there was no port
or service included in the name, but not when there was one.

This commit adds a check for the missed case. I believe that it would
work just as well to replace the nil passed for the user in the call
two lines down, but I wasn't completely sure I wasn't missing
something for why nil was passed in that case so chose the
conservative option of adding the more specific check for the case
when both user and port are supplied.
@kamidon
Copy link
Contributor Author

kamidon commented Apr 17, 2019

This is now rebased on master, handles auth-source-pass-port-separator, and has an additional test for the custom port separator case.

@DamienCassou DamienCassou merged commit 6f2c12b into DamienCassou:master Apr 17, 2019
@DamienCassou
Copy link
Owner

Awesome work, thank you!

@DamienCassou
Copy link
Owner

For some reason, I didn't merge your commit but a commit I created with your code. I don't know what happened except that I pushed your branch to my repository. You are still marked as the author but the commit is me. Sorry about that.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants