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

CAMEL-12723 - camel-ftp add IPv6 support #2477

Closed
wants to merge 3 commits into from

Conversation

onderson
Copy link
Contributor

the second commit makes PR less sensitive because the change in URISupport in the first commit have higher impact even though all UTs are OK without this second commit.

@davsclaus
Copy link
Contributor

I am not too fond with this where there is a ftp hack like this. You can use IPv6 addresses in hostnames in other components as well (however not so common, and IPv6 are a more complex syntax than good old IPv4)

@onderson
Copy link
Contributor Author

from coding perspective, i dont like it too.
the main issue here is that enpoint uri syntax and url syntax seems clashing in case of handling their encoding and using their uri/url values in the component's endpoint creation.
unless playing around with lots of utility classes doing normalization/encoding and what not during processing of endpoint string definition and unless coping with high impact, i felt like hard to do a fix.

also please note that that hack is not necessary. (i added this to only affect ftp component). only the first commit is enough without the hack, based on the unit tests. however, i add the second commit defensively not to cause anything subtle in other components extending DefaultComponent.

and another side note that we have a little bit of similar hack for http based components :)

There is a workaround as you mentioned already. we can drop the PR if everybody agrees and put such fixes off till 3.0

@onderson
Copy link
Contributor Author

closing.

@onderson onderson closed this Aug 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants