Fix malformed template, cache CelesTrak TLEs #28
Conversation
This was supposed to be two commits but a wayward --amend means these get squashed: - Make sure that we can load the template config once all the template text has been replaced. The Hosts.station field default wasn't updated in the template when the representation changed in Config so the template was generating invalid values. This adds a test to try and prevent that in the future and fixes the direct issue. - Implement TLE caching for CelesTrak. CelesTrak asks that we access their API no more than once every two hours. During normal operation our access is dictated by when a pass happens which isn't really more than once or twice a day, but during testing or experimenting we could fetch a TLE on every start up. This also affects new users who wouldn't initially have a TLE or know how to get one. The method employed here mirrors how skyfield does it but I chose not to use their api directly for easier testing. In the future after the Pydantic change this should probably be redone, possibly to append to the pass_commander.toml TleCache.
| contents = contents.replace('<', '') | ||
| contents = contents.replace('>', '') | ||
| with path.open('w') as f: | ||
| f.write(contents) |
There was a problem hiding this comment.
If we generate a template then immediately overwrite the angle brackets while testing the config, wouldn't that just add an unnecessary step. Unless the angle brackets really are necessary to explain the what is expected to be in a field of the config file to the user.
There was a problem hiding this comment.
Stripping the angle brackets and having it work is a bit of a hack admittedly. We would really like the user to enter their specific values for the fields marked in such a way though, and words inside the brackets tell the user what to do. The main one is the station name, obviously we don't know what the user's station is and for regulatory reasons they really should have some kind of name specific to them.
I could probably rethink the whole angle bracket system when the configs are overhauled as part of the pydantic conversion. Maybe just not filling out some mandatory fields and then have pass-commander yell about which fields are missing on startup would be better.
There was a problem hiding this comment.
Missing fields and possibly only specific characters you will be expecting would be good as well.
| if tle is None and filename.exists(): | ||
| logger.info("using cached TLE from %s", filename) | ||
| lines = filename.read_text().splitlines() | ||
| if lines[0] == "No GP data found": |
There was a problem hiding this comment.
Why are we looking for this specific string? Is the error message, "No GP data found", from a specific place were you can get TLEs?
There was a problem hiding this comment.
Yep, when the celestrak request above doesn't find the satellite we're looking for it responds with this string. It's not in the API docs (the link above) but it always appears to respond with it. We save it to a file because any result, successful or not, should follow their 2 hour timeout caching policy.
There was a problem hiding this comment.
We should probably make note of that, in case they randomly decide to change it.
erfi-x2
left a comment
erfi-x2
left a comment
There was a problem hiding this comment.
Everything looks good. Outside of the areas of my comments I can't find anything else that I can find to at least question. Thanks for giving me the chance to review this PR!
This was supposed to be two commits but a wayward --amend means these get squashed:
Make sure that we can load the template config once all the template text has been replaced. The Hosts.station field default wasn't updated in the template when the representation changed in Config so the template was generating invalid values. This adds a test to try and prevent that in the future and fixes the direct issue.
Implement TLE caching for CelesTrak. CelesTrak asks that we access their API no more than once every two hours. During normal operation our access is dictated by when a pass happens which isn't really more than once or twice a day, but during testing or experimenting we could fetch a TLE on every start up. This also affects new users who wouldn't initially have a TLE or know how to get one. The method employed here mirrors how skyfield does it but I chose not to use their api directly for easier testing. In the future after the Pydantic change this should probably be redone, possibly to append to the pass_commander.toml TleCache.