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

Use erlang-idna from hackney instead of own impl. #350

Merged
merged 1 commit into from Jan 15, 2015
Merged

Use erlang-idna from hackney instead of own impl. #350

merged 1 commit into from Jan 15, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jan 13, 2015

Removes maintenance burden of idna and also enables the use of hackney when developing own modules (otherwise there's a conflict because both modules are called the same).

I'm not sure if everything works though, I couldn't get the tests to run (neither on the local machine nor on TravisCI). Therefore if you think something's off please tell me and I'm more than happy to change the PR (that's why the branch is also called WIP).

Using case catch is probably not the best idea as well and I guess you won't like it much. Please guide me in implementing a proper way to do this.

I hope you agree on what the PR tries to achieve though & I'm looking forward to your comments.

@michalwski
Copy link
Contributor

Your PR is useful and I'd like to merge it to the master branch but there is a problem. Every job on travis fails because s2s_SUITE fails. Unfortunately there is no crash on Mongoose side while doing the s2s tests. In the case catch I'd log the error and see what does it tell. Maybe the new idna module returns data in some other format or expects data in some other format.

In your tests (on your travis) I also noticed that bosh_SUITE failed. That's because recently we fixed an issue with bosh and added test case for that. So travis used tests from master branch (with the new test case) but your MongooseIM was forked before the fix and that's why bosh_SUITE failed. You have 3 options to fix this:

  1. configure your travis to use your fork of ejabberd_tests.
  2. fix esl/ejabberd_tests in rebar.config to checkout master branch before the bosh fix.
  3. use newest Mongoose

@ghost
Copy link
Author

ghost commented Jan 13, 2015

Thanks @michalwski, will see what the error is & merge upstream master back into the branch. I anyway planned doing a new PR once everything was clear in order to have one commit only for merging.

Will get back to you as soon as I made sure s2s_SUITE runs again!

@ghost
Copy link
Author

ghost commented Jan 13, 2015

s2s_SUITE worked now, there are some other failures though. I'll wait on the build and see what happens.

I have my doubts about having the same code 3 times with handling of the exception & converting to binary again. Maybe I should move this into a simple function somewhere. What do you think?

@ghost
Copy link
Author

ghost commented Jan 14, 2015

Perfect, seems now to work :) What about creating a helper function? Do you think it's necessary?
If not I'll create a new PR where I'll merge all commits into one.

@michalwski
Copy link
Contributor

Right, helper function is good idea :)

Regarding other fails. Some times in rush hours where travis-ci is very loaded it happens that some time sensitive tests fails. Usually it's 1-2 jobs where such test fails.

@ghost
Copy link
Author

ghost commented Jan 15, 2015

Where would you place it? adding a idna module would defeat at least one of the purpose of this PR ;)

@michalwski
Copy link
Contributor

I think ejabberd_s2s is a good candidate.

@ghost
Copy link
Author

ghost commented Jan 15, 2015

Ok, thought about this as well, will do 👍

@michalwski
Copy link
Contributor

Looks good now :) You don't have to create new PR, just rebase your branch where you squash some commits and force push to the same branch. And remove WIP from the PR's name :)

Removes maintenance burden of `idna` and also
enables the use of `hackney` when developing own
modules (otherwise there's a conflict because both
modules are called the same).
@ghost ghost changed the title WIP: Use erlang-idna from hackney instead of own impl. Use erlang-idna from hackney instead of own impl. Jan 15, 2015
@ghost
Copy link
Author

ghost commented Jan 15, 2015

Done 👍 (was a bit afraid, force push and such.. but went apparently well ;))
Thanks for your help.

@michalwski
Copy link
Contributor

Thanks for your work! I'll wait for travis to finish the build and I'll merge to master.

michalwski added a commit that referenced this pull request Jan 15, 2015
…pendency-wip

Use erlang-idna from hackney instead of own impl.
@michalwski michalwski merged commit b81c610 into esl:master Jan 15, 2015
@ghost
Copy link
Author

ghost commented Jan 15, 2015

Yay 👍

@ghost ghost deleted the remove-idna-module-and-use-dependency-wip branch January 15, 2015 12:18
This was referenced Apr 2, 2015
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.

1 participant