feat!: add support for HTTP Proxy

[llamafilm]

This is my first attempt at supporting an external HTTP Proxy, based on Tesla's documentation. It's working for me on a 2021 Model 3, in conjunction with the Tesla HTTP Proxy add-on.

The new Fleet API uses VIN for all commands instead of ID. The wording here implies that this should work for older vehicles too, but I need someone else to test if this works on older vehicles.

New optional parameters:

• client_id is required by the new API. I don't understand why this library previously had client_id and client_secret hardcoded but I left that part as-is if this config field is blank.
• api_proxy_url should point to an HTTPS domain
• api_proxy_cert if using a self-signed SSL certificate for this, you can provide the path to the certificate here so it will be trusted

Closes #452.

[thierryvt]

I noticed that in a lot of places the vehicle id is simply swapped for the vin, seems to me like this will break backward compatibility for cars who can remain on the old owner API. Especially given that Tesla is most likely planning on charging money for fleet API access sometime down the road.

Perhaps we could work with an additional flag to indicate whether to use the vehicle id with the owner API or the vehicle VIN with the fleet API? Or perhaps just base ourselves on whether or not the custom API url is provided.

I don't understand why this library previously had client_id and client_secret hardcoded but I left that part as-is if this config field is blank.

Might simply have been grabbed from the mobile app. Tesla is not gonna create a new client_id and client_secret for each person who install their mobile app. Probably.

[llamafilm]

I thought about that, but I’m hoping VIN works for Owner API too. Tesla’s stated purpose for this proxy is to let legacy apps keep working with no code changes. And there is a note on this page:

Legacy clients written for Owner API may be using a vehicle's Owner API ID when constructing URL paths. The proxy server requires clients to use the VIN directly, instead.

That sounds to me like Owner API can use either one. So I’m hoping VIN works there too. I need someone else to test this with an older vehicle.

[thierryvt]

The Fleet API documentation does mention the 'vehicle_tag' can be either the vin or the ID.: https://developer.tesla.com/docs/fleet-api#vehicle-commands

Combined with the fact that the new command SDK requires the vin and the word 'may' it does make sense...

So either we find someone with an older Tesla or we play it safe and include the safeguards anyway?

[llamafilm]

I think @alandtse has an older Tesla; maybe they can help test?

[alandtse]

I do not. You'll need to recruit someone or just make it a flag to use the new value.

[thierryvt]

I created a pr to add the flag (https://github.com/llamafilm/teslajsonpy/pull/1) onto your fork of teslajsonpy @llamafilm as to not clutter this repo with multiple trying to achieve basically the same thing.

Did not add/update tests yet though, I'll do that tomorrow.

edit: added some tests, might still be useful to add some for the controller module as well but I'm out of time for today.

[llamafilm]

Thanks for your effort, but I would prefer not to add complexity if it’s not necessary. I emailed Tesla about this and they confirmed VIN should work:

Hi Elliot, Owner API supports both the API ID and VIN. If you switch to using vin, you should see no change to functionality. Best, Fleet API Support

[thierryvt]

I agree but I didn't feel like waiting for a random person with an older car to try it out and hadn't considered simply mailing Tesla. Guess my contribution to the project is not needed 😂

Seems like you got it covered, if you need help with anything I've got some extra time tomorrow, after that not so much.

[llamafilm]

I've never used pytest and I don't have time to learn it right now, so your contributions there are appreciated! Would you like to redo the PR to just focus on tests? It's currently failing this one, which doesn't seem related to this change...

FAILED tests/unit_tests/test_exceptions.py::test_custom_wait - assert 1.0000000000000142 <= 1

[thierryvt]

Sure, I'll take a look.

[thierryvt]

Or I would if the test failed for me. It did yesterday, but now it's fixed itself. At least on my machine. The only thing I did was upgrade a couple dependencies and my python version to get the build working on my (windows) machine but hadn't restarted yet so maybe something stuck around in cache that got cleared with a reboot?

I'll make the PR, but it might be a good idea if you tried the changes first before merging to confirm it's working.

[alandtse]

Please note I'm waiting for this to come off draft status before merging. Once merged, this is a breaking change and will result in a new major version number. Please make sure the instructions are updated in https://github.com/alandtse/tesla. I don't know how to use this proposed PR and will rely on @llamafilm to handle the HA addon integration changes

[llamafilm]

I hope this doesn’t break anything. Can you explain what issues you’ve seen?

[alandtse]

You are changing function behavior (e.g., https://github.com/zabuldon/teslajsonpy/pull/453/files#diff-3eb702ef3ee0ab4447877d75375b8204dd4ca6e9911dc8e24746e27867c968caR463). It is a breaking API change since any code using those functions will have to be changed or at least review.

[thierryvt]

@alandtse If we want to avoid changes to function signatures then I do still have my branch, for which I should be able to create a PR directly onto this repo without issue. It does not change any of the signatures (except for the TeslaCar constructor) but, as mentioned above, it does add a bit of complexity because it determines whether to use the vin or car_id through the use of an extra flag.

I could update my code to avoid the added complexity of the flag by just defaulting to the VIN (which should work as per Teslas email) and still avoid changing any signatures.

[alandtse]

I mean breaking changes are fine given the major change at Tesla. I have no emotional attachment to saying it's breaking or not. It's just the nature of the change. If you get a PR in on top to ensure the old functionality works without change, that's fine too.

[thierryvt]

It's about finding the best way forward, and I'm of the opinion that if we can avoid (major) breaking changes then we should at least consider it. Which is why I think my approach still has value.

I'll re-open the PR with the vin flag but on this repo directly, and I'll do one without the flag (so defaulting to VIN). See what we like more. If the conclusion is then still that we like Llamafilm's approach best then so be it.

[alandtse]

closed by #454