Skip to content

Conversation

@cartercanedy
Copy link
Collaborator

@cartercanedy cartercanedy commented May 28, 2025

The semantics of ads::Source::Auto and the documentation seemed unclear with respect to configurations where there is no local router and the client is connecting directly to the remote ADS server or the client is using a local router to connect to a remote machine.

This PR updates both the documentation and adds a new Source::AnyLocalIp variant (which behaves identically to Source::Auto) for the core library, and updates adstool route so that remote route management functions properly with more edge-cases.

note: this PR is stacked with #39, will rebase to get rid of the extra commits once it's merged

@cartercanedy cartercanedy force-pushed the source-fixes branch 2 times, most recently from be6bc41 to 0e25676 Compare June 1, 2025 22:20
@cartercanedy cartercanedy changed the title Draft: Refactor of and update documentation for Client code && apply changes to make adstool route ... work for remote machines Draft: Refactor ads::client::{Client, Source} && apply fixes to adstool to better handle edge cases Jun 2, 2025
@cartercanedy cartercanedy force-pushed the source-fixes branch 2 times, most recently from 860c1a5 to aa341b8 Compare August 19, 2025 16:15
@birkenfeld
Copy link
Owner

If you don't mind I'll review this once the other PR is merged :)

In general, I don't understand the reason to rename Auto to Any well enough; if it's intended to make it more clear, wouldn't something like LocalIp be more descriptive?

@cartercanedy
Copy link
Collaborator Author

wouldn't something like LocalIp be more descriptive?

yep, changed it

@birkenfeld
Copy link
Owner

Thanks, but I'm still not sure why "any" local IP? It's exactly the local IP isn't it?

Do you think it's also a good idea to provide something like LocalIPWithRouteAdded? Or too much feature creep and better to let users call add_route themselves?

@cartercanedy
Copy link
Collaborator Author

Honestly, I'm looking back at this PR and I can't remember how to reproduce the auth issues that I was seeing. I don't really think that there's a whole lot here besides some minor fixes that I can cherry-pick into their own separate PRs. Let me know what you think @birkenfeld, but I'll get started working on breaking up the changes into smaller diffs in the meantime

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.

2 participants