bwoodworth / hassio-addons

11 stars 9 forks source link

Account for SL password #11

Closed resistr closed 4 years ago

resistr commented 4 years ago

Changed to account for SL Password. Uses new config option for password (room for improvement to use secrets). Still works without a password.

bwoodworth commented 4 years ago

@resistr Can you test the latest release. I'm not sure your updates are working.

resistr commented 4 years ago

I have it up and running presently. It's my friends pool; so I have to schedule it with him; but I am meeting up with him in a couple of hours so likely I can re-test everything this afternoon / evening. I assume you mean not working without a password? is there an error or a log? I may be able to spot it sooner with more info. Either way; I certainly don't want to leave you hanging so I'll be on it as soon as I can get permission.

bwoodworth commented 4 years ago

Actually, I mean with a password. I'm confused about line 6 of initialize.js. process.env.ScreenLogic_password is coming back undefined for me. I was thinking I could simplify my code if its possible to pull the config values that way, but it didn't work so I did some more testing and found that it is undefined. Should we be able to pull the HA config values using process.env?

resistr commented 4 years ago

Ok, cool. Yea it certainly is working with the password and yes the passing of the info should be able to be simplified; runs.sh sets the environment variable. With no password in the config we want it to be null. With a password in the config the environment var should become the password. Then looking at the UnitConnection constructor there is not a separate constructor for with and without password so passing two params is the same as passing three params if the third param is undefined.

You could add some verification code locally to see it in action:

On line 17 or run.sh add: FOO="BAR"

On Line 7 of initialize.js add: console.log('FOO=' + process.env.FOO);

You should see FOO=BAR in the log when it re-initializes.

I'll attach screenshots from my friends running environment.

I must apologize for introducing a different way to do things. Let me know if I can provide any more info. I didn't change the way the existing configuration worked because I didn't want to create more risk, and I didn't want to follow the existing argument method because the positions wouldn't have been the same for all scripts; I also perceived that to be more risk of making a mistake. But it is certainly working with the password.

Home Assistant Pentair Validation.pdf

bwoodworth commented 4 years ago

Strange. I am getting undefined using your example:

image

resistr commented 4 years ago

That is odd. I'm sorry that you have time / desire to work on it now and this is hanging you up. Unfortunately at this point; I'll have to talk to my friend and run through things this evening. I'll get back to you as soon as I can.

bwoodworth commented 4 years ago

No worries. It is at least working as is. I was just hoping to simplify the code. I will play around with it a bit more. Thanks for the help.

resistr commented 4 years ago

Ok I think I now understand the disconnect. Reviewing https://developers.home-assistant.io/docs/add-ons/configuration it appears my change the SH was not necessary. I don't have a deep enough understanding of bash; but I suspect the set -e is why my sample above didn't work...

This should though.

In initialize.js:

console.log('Screen Logic Server from env=' + process.env.ScreenLogic_server);

It appears the underlying docker image is processing the configuration into the environment variables prior to ever passing control to run.sh.

That certainly would simplify the individual scripts and the run.sh to leverage the config in that way :)

Let me know if you have anymore issues I'll be around tomorrow and should be able to help.

bwoodworth commented 4 years ago

Yeah, that isn't working either. Can you confirm you are not getting undefined when you add this to initialize.js?

console.log('Screen Logic Server from env=' + process.env.ScreenLogic_server);

bwoodworth commented 4 years ago

I figured it out. You need the following to send the environment variable to node:

export KEY=value

resistr commented 4 years ago

So, yes that's what I came to as well. I ultimately am left with more questions than answers; but I've been tinkering with it all day. I'll be submitting a PR to remove the changes ultimately is what it ultimately boils down to. With a little egg on my face :) I must have configured something wrong the first pass but locally even a password protected SL will allow a passwordless connection; so these changes weren't necessary; but educational I guess.

bwoodworth commented 4 years ago

Interesting findings. So the password isn't necessary at all even if the ScreenLogic is password protected?

I have made a ton of changes in the last few days so your PR probably isn't necessary. I can strip out the password stuff if we don't need it.

resistr commented 4 years ago

Correct; if connecting locally, even to a password protected SL the password does not seem to be required. PR is there, because git's revert actually worked for once; but just abandon it if you want to strip it manually and not deal with conflicts.

bwoodworth commented 4 years ago

Would it be worthwhile to keep it in just in case someone wants to connect to a remote SL?

resistr commented 4 years ago

I don't believe so; I think the connection process is different enough that it would end up looking different than this does.