Catch duplicate API key specified (plus scope creep) #226
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
🤣🤣🤣 |
|
Long may the changes in the last two PRs hide dormant. Prepped for next anyways. |
While I was reviewing and approving your PR, I was sitting in the Palais waiting for...
|
|
Here's an example for you of how smart Copilot can be. Scenario: I've copied a unit test function for invalid API limit, and am modifying it to test for invalid hard limit. 
|
|
We now meet (well, far exceed) the minimum requirements for becoming a core component. Unit testing of config_flow.py is must-ride. I worked out how to shoehorn pytests for a custom into the dev container environment, with minimal alterations required when transitioned as a core component... In "mounts": [
"source=${localEnv:HOME}/Documents/GitHub/ha-solcast-solar/custom_components/solcast_solar,target=${containerWorkspaceFolder}/config/custom_components/solcast_solar,type=bind",
"source=${localEnv:HOME}/Documents/GitHub/ha-solcast-solar/tests,target=${containerWorkspaceFolder}/tests/components/solcast_solar,type=bind",from |
autoSteve
changed the title
OK, I'll try to have a look over the weekend. Very tired, now, so off to bed, but maybe we throw it at the wall and see if it sticks. Will read up on the process. |
|
Sounds like a journey, but I'm betting it's a shorter one than HACS at the mo'... We got this. |
|
I've removed the self-signed certs to stop GitGuardian complaining about them. The sim creates the cert if required on startup. |
|
The hard limit validation oopsie would have been caught by the unit test had I also looked for an expected positive outcome going wrong instead of expecting the worst from bad input. I did not. The unit test failed everyone. It sailed through without a care in the world. It does not now. |
|
So I am kind-of guessing re. #227. I am only led by the exception. The latest commit catches the very first exception regarding 'index' and then bluntly refuses to start the integration with an inferance of a corrupt I thought about doing something fancier, but fancier would have required a LOT more input as to the situation, and a LOT more testing. I am not in the mood for testing. And I have no input. So it is what it is for now. |
I am kind-of guessing that some ancient installs - possibly manual, non-HACS, have been upgraded, and they are the cause of the last few issues like this. |
|
Kind-of not, I'm tipping. The log indicated a usage cache load, with a count of two for API used, so must be a relatively recent version. Last reset was within twenty-four hours. This might have been from a prior start attempt, but there are two sites, so if anything API used should be four calls, given history load on fresh start. Unless the second site failed to load... I got nothin' but conjecture. |
Ahh, I did squint at the log on my phone after I carefully picked up my specs case, left home, and discovered that I had left me specs at home, but didn't spot that. |
|
Further fun with unit tests. Just uncovered a bug: Turning off detailed half-hourly attribute with detailed hourly enabled results in a puff of virtual magic smoke. |
|
In this commit is migration of usage. On key change there should be Test coverage still needs work, but there are bloody few lines that miss after all that lot... |
|
(You could hack together a test for your prod rig by editing |
|
OK, got a slightly odd scenario, here.
It looks like the API change is not resulting in a reauth. Log: |
|
Prod test successful:
Log: |
|
The sites comparison is failing for your dev container, so initial cache load is refused. That should not have happened, as your sites have not changed, and the cache should be used. Thinking about this situation the integration should fail hard as it did correctly if the sites are different, but re-auth should definitely be shown. Startup may not be raising that HA exception. I will try to replicate using the same start sequence. (This scenario may not currently be covered by the PyTest suite, but will check whether I coded it. I am definitely testing the forecast fetch failure sequence, and there is 100% coverage again, so it would suprise me if it's not.) Good news for the more common scenario of 403 on forecast fetch in prod! |
|
This scenario was not covered by the PyTest suite. It is now. Code still has startup issues. |
|
Noticed something funky - dunno if it is HA or a side effect from changing how config is stored - will look into it, but figured I'd test the latest code by editing the API key in core.config_entries in my Dev Container. I changed the Just wondering if this might be the reason why you're seeing double restarts and other odd behaviour - two different sets of data stored - options and data? I will remove the integration and clean up old entries to explore more, but thought I'd post what I saw here. {
"version": 1,
"minor_version": 4,
"key": "core.config_entries",
"data": {
"entries": [
{"created_at":"2024-11-05T22:33:49.065844+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"sun","entry_id":"01JBZ7CX892Q3CHBQ8VM7GS1HD","minor_version":1,"modified_at":"2024-11-05T22:33:49.065851+00:00","options":{},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"import","title":"Sun","unique_id":null,"version":1},
{"created_at":"2024-11-05T22:48:54.978705+00:00","data":{"token":"redacted"},"disabled_by":null,"discovery_keys":{},"domain":"hacs","entry_id":"01JBZ88HY2DAX0811MTMJXHG5G","minor_version":1,"modified_at":"2024-11-05T22:48:54.978725+00:00","options":{"experimental":true},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"","unique_id":null,"version":1},
{"created_at":"2024-11-19T23:08:45.651933+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"epa_victoria_air_quality","entry_id":"01JD3AYYPKNCF1ZNW6BCBGS34Q","minor_version":1,"modified_at":"2024-11-19T23:08:45.651939+00:00","options":{"api_key":"redacted","site_id":"4afe6adc-cbac-4bf1-afbe-ff98d59564f9"},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"EPA Air Quality","unique_id":null,"version":2},
{"created_at":"2024-11-28T03:53:59.551705+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"spook","entry_id":"01JDREEZFZCA3WRRD4S94P9P23","minor_version":1,"modified_at":"2024-11-28T03:53:59.551714+00:00","options":{},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"Your homie","unique_id":null,"version":1},
{"created_at":"2024-12-10T05:30:40.496122+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"sew_usage","entry_id":"01JEQGRMFGVH6BJNY8MW320DJN","minor_version":1,"modified_at":"2024-12-10T05:30:40.496132+00:00","options":{"browserless":"http://192.168.1.200:3000","install_date":"2024-12-10","mains_water_serial":"redacted","password":"redacted","recycled_water_serial":"","token":"redacted","username":"redacted"},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"South East Water Usage","unique_id":null,"version":1},
{"created_at":"2025-01-13T00:57:50.462204+00:00","data":{"api_key":"redacted - old key that I changed this morning","api_quota":"10","attr_brk_detailed":false,"attr_brk_estimate":true,"attr_brk_estimate10":true,"attr_brk_estimate90":true,"attr_brk_halfhourly":true,"attr_brk_hourly":true,"attr_brk_site":true,"auto_update":1,"customhoursensor":1,"damp00":1.0,"damp01":1.0,"damp02":1.0,"damp03":1.0,"damp04":1.0,"damp05":1.0,"damp06":1.0,"damp07":1.0,"damp08":1.0,"damp09":1.0,"damp10":1.0,"damp11":1.0,"damp12":1.0,"damp13":1.0,"damp14":1.0,"damp15":1.0,"damp16":1.0,"damp17":1.0,"damp18":1.0,"damp19":1.0,"damp20":1.0,"damp21":1.0,"damp22":1.0,"damp23":1.0,"hard_limit_api":"100.0","key_estimate":"estimate","site_damp":false},"disabled_by":null,"discovery_keys":{},"domain":"solcast_solar","entry_id":"01JHEJNG3Y8Y7VTE1KNGQKX4K5","minor_version":1,"modified_at":"2025-01-19T23:45:20.063590+00:00","options":{"api_key":"redacted new key that I created this morning","api_quota":"10","attr_brk_detailed":false,"attr_brk_estimate":true,"attr_brk_estimate10":true,"attr_brk_estimate90":true,"attr_brk_halfhourly":true,"attr_brk_hourly":true,"attr_brk_site":true,"auto_update":1,"customhoursensor":1,"damp00":1.0,"damp01":1.0,"damp02":1.0,"damp03":1.0,"damp04":1.0,"damp05":1.0,"damp06":1.0,"damp07":1.0,"damp08":1.0,"damp09":1.0,"damp10":1.0,"damp11":1.0,"damp12":1.0,"damp13":1.0,"damp14":1.0,"damp15":1.0,"damp16":1.0,"damp17":1.0,"damp18":1.0,"damp19":1.0,"damp20":1.0,"damp21":1.0,"damp22":1.0,"damp23":1.0,"hard_limit_api":"100.0","key_estimate":"estimate","site_damp":false},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"Solcast Solar","unique_id":null,"version":14}
]
} |
|
Given I know exactly what was causing the double starts, no. The root was switching to a The doubling of things in the entry are because of me. Nicely spotted. There are two The snip below is what happens when the integration is first configured, so "data" should be empty. I think the code that was causing the double reload did it because I'm fairly certain I mistakenly set "data" in that call. With latest code data sould be empty, but it won't get emptied if it's there right now unless the integration is re-set up. If you want to manually hack it, delete all keys in "data", and change the key in "options". |
I deleted everything solcast from the config folder, and from core.config_entries, core.device_registry, and core.entity_registry: New core.config_entries looks just fine: {
"version": 1,
"minor_version": 4,
"key": "core.config_entries",
"data": {
"entries": [
{"created_at":"2024-11-05T22:33:49.065844+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"sun","entry_id":"01JBZ7CX892Q3CHBQ8VM7GS1HD","minor_version":1,"modified_at":"2024-11-05T22:33:49.065851+00:00","options":{},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"import","title":"Sun","unique_id":null,"version":1},
{"created_at":"2024-11-05T22:48:54.978705+00:00","data":{"token":"redacted"},"disabled_by":null,"discovery_keys":{},"domain":"hacs","entry_id":"01JBZ88HY2DAX0811MTMJXHG5G","minor_version":1,"modified_at":"2024-11-05T22:48:54.978725+00:00","options":{"experimental":true},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"","unique_id":null,"version":1},
{"created_at":"2024-11-19T23:08:45.651933+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"epa_victoria_air_quality","entry_id":"01JD3AYYPKNCF1ZNW6BCBGS34Q","minor_version":1,"modified_at":"2024-11-19T23:08:45.651939+00:00","options":{"api_key":"redacted","site_id":"4afe6adc-cbac-4bf1-afbe-ff98d59564f9"},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"EPA Air Quality","unique_id":null,"version":2},
{"created_at":"2024-11-28T03:53:59.551705+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"spook","entry_id":"01JDREEZFZCA3WRRD4S94P9P23","minor_version":1,"modified_at":"2024-11-28T03:53:59.551714+00:00","options":{},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"Your homie","unique_id":null,"version":1},
{"created_at":"2024-12-10T05:30:40.496122+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"sew_usage","entry_id":"01JEQGRMFGVH6BJNY8MW320DJN","minor_version":1,"modified_at":"2024-12-10T05:30:40.496132+00:00","options":{"browserless":"http://192.168.1.200:3000","install_date":"2024-12-10","mains_water_serial":"redacted","password":"redacted","recycled_water_serial":"","token":"redacted","username":"redacted"},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"South East Water Usage","unique_id":null,"version":1},
{"created_at":"2025-01-20T04:56:10.305042+00:00","data":{},"disabled_by":null,"discovery_keys":{},"domain":"solcast_solar","entry_id":"01JJ112XT017FVGCF0S84VMBM0","minor_version":1,"modified_at":"2025-01-20T04:56:10.538323+00:00","options":{"api_key":"redacted","api_quota":"10","attr_brk_detailed":false,"attr_brk_estimate":true,"attr_brk_estimate10":true,"attr_brk_estimate90":true,"attr_brk_halfhourly":true,"attr_brk_hourly":true,"attr_brk_site":true,"auto_update":1,"customhoursensor":1,"damp00":1.0,"damp01":1.0,"damp02":1.0,"damp03":1.0,"damp04":1.0,"damp05":1.0,"damp06":1.0,"damp07":1.0,"damp08":1.0,"damp09":1.0,"damp10":1.0,"damp11":1.0,"damp12":1.0,"damp13":1.0,"damp14":1.0,"damp15":1.0,"damp16":1.0,"damp17":1.0,"damp18":1.0,"damp19":1.0,"damp20":1.0,"damp21":1.0,"damp22":1.0,"damp23":1.0,"hard_limit_api":"100.0","key_estimate":"estimate","site_damp":false},"pref_disable_new_entities":false,"pref_disable_polling":false,"source":"user","title":"Solcast Solar","unique_id":null,"version":14},
]
}
}Will get back to testing other stuff including API key rotation. |
|
Perfect. |
|
And tested key rotation with HA shutdown - works nicely with your latest (approx 3:15) commit. |
|
One more thing - I'm not sure if this is my configuration, or code changes, but I think it is code changes: 4.2.7
4.2.8 (with same configuration)
Noticed because
|
|
Interesting. I can't replicate that in dev, plus with data that bad PyTest would spew it's guts. SFTP is currently broken for me to transfer files to prod, so I'll need to fix that and get the code running somewhere real. 
|
|
The ziggly line would be your updates getting fwesh forecasts from Solcast I would think. Looks like ten adjustments from sunrise to sunset, so sensor updates are deffo working!. I'll try those exact settings in the dev container. |
|
That's a bona fide bug. 
|
|
I'm actually really embarrassed about that one. Dicked logic. Fixed the options flow translatable confirmation message while I was down there. I believe we have everything seen in the UI translatable. As I find more I fix more, but haven't found one since the options flow. |
I had noticed that, but hadn't checked if the issue was that I hadn't successfully copied the translations or refreshed the cache, so hadn't mentioned it - yet 🤣. |
|
Note: also fixes #233 |
And shift some exceptions for testing. Sensors disabled are day 3-7 forecast and the custom X hour sensor.
|
The day 3-7 sensors, and the custom X hour sensor can't be used by many folk, so now they are disabled by default for a new install. There should not be any change for existing installations. My standards are slipping... I'll think about that tomoz. |
|
(Commit fail. BTW, as an example, technically this lot does not pass "bronze". Services are not set up in |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why anyone of sound mind would do this is beyond me. But it does result in exceptions that want heading off at the pass...