-
Notifications
You must be signed in to change notification settings - Fork 16
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
Add snowflake support #63
Conversation
stevenmcdonald
commented
May 12, 2023
•
edited
edited
- Add snowflake support to Envoy Java code
- Add socks5 param for envoy:// URLs to Cronet Java code
ampCache = queryParts[1] | ||
} else if (queryParts[0].equals("front")) { | ||
front = queryParts[1] | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forgot to allow overriding the ice
value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this comment is from before if (queryParts[0].equals("ice"))
was added, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it matter? It's 3 weeks old, I would assume I didn't put that in after the feature was added... I think it was meant to be a reminder
…into snowflake-support
not sure if i should just approve this myself, but the patch changes makes sense to me and i worked on most of the kotlin code. |
|
||
// borrowed from the list Tor Browser uses: https://gitlab.torproject.org/tpo/applications/tor-browser-build/-/merge_requests/617/diffs | ||
// too many is a problem, both of these seem to work for us | ||
var ice = "stun:stun.l.google.com:19302,stun:stun.sonetel.com:3478,stun:stun.voipgate.com:3478" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should these be moved into the credentials.properties?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, but as an optional parameter in the envoy url. this is basically the default value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those are defaults. Apparently I forgot to updated the comment when I added a third
nothing jumps out at me other than potentially moving the STUN urls into the secrets |
merging this now, although it won't build until the 150-1 cronet dependency is available in maven |