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.

Sign up for GitHub

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

Jump to bottom

custom server plugin - fixes for ddns update server url parsing #150

Merged
merged 1 commit into from Nov 30, 2016
Merged

custom server plugin - fixes for ddns update server url parsing #150

merged 1 commit into from Nov 30, 2016

Conversation

sigmoidal
Copy link
Contributor

I've redone the patch for you. It should be cleaner and more consistent with the code style too.
Let me know if you need any clarifications.

I'm testing it for my own ddns and it seems to work fine so far.

troglobit
troglobit approved these changes Nov 30, 2016
View reviewed changes
Copy link
Owner

@troglobit troglobit left a comment

Choose a reason for hiding this comment

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

Looks much better, thanks! Only some minor ) and too long indentation of args in split line arg lists. But I can fix that myself in a follow-up commit 👍

I'm not doing any testing, since I assume you've tested it, or have successful results so far 😃

@troglobit
Copy link
Owner

For future reference, about commit messages, see http://chris.beams.io/posts/git-commit/ for an excellent guide.

(I should create a file CONTRIBUTING.md like I have in some other projects to help guide contributors :)

@troglobit troglobit merged commit 5bb76b7 into troglobit:master Nov 30, 2016
@sigmoidal
Copy link
Contributor Author

Thanks for merging this in. Agreed, next time I'll write a better commit comment.

@troglobit
Copy link
Owner

No problem 😃

... btw, just added my own fixups, including a very minor simplification/refactor that I believe do the
same as your code. More eyes on the code etc., comments welcome!

@Cadence-GitHub
Copy link
Contributor

I think this PR broke something. When I specify something like this:
ddns-path = "/update?hostname=%h"
The resulting URL that is being sent to the server looks like this:
%2fupdate%3fhostname=test.example.com

I think it is incorrectly encoding the URL

@troglobit
Copy link
Owner

troglobit commented Feb 5, 2017 •
edited

@Cadence-GitHub Hmm, yeah that doesn't look right. I think it's the URL encoding I added in 02e22f2, for issue #152. I'll look into it!

troglobit added a commit that referenced this pull request Feb 5, 2017
@troglobit
Issue #152: Followup to 02e22f2, no URL encoding of /, ?, and &
34dceda 
As mentioned in issue #150 by @Cadence-GitHub, the full server path
cannot be encoded.  A DDNS path like this

    ddns-path = "/update?hostname=%h"

would result in this URL being sent to the server:

    %2fupdate%3fhostname=test.example.com

Which is not what we want.  This patch adds exceptions for the
above mentioned 'special' characters.

Signed-off-by: Joachim Nilsson <troglobit@gmail.com>
@troglobit
Copy link
Owner

@Cadence-GitHub I just pushed a fix in 34dceda which I think will fix this problem. Thank you for the heads-up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@troglobit troglobit troglobit approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@sigmoidal @troglobit @Cadence-GitHub