Refactor Game Management

[techman83]

This is a refactor of the game management to introduce Multi Game support. Whilst there are a large number of lines added, about a 1/3 are test cases and of the code changed, there is now 79% coverage and increased overall coverage from 54% to 67%.

The configuration uses the multi var Click option to achieve the multi game config, which is done via --option var --option var2 or a space separated env var OPTION=var var. This allows a configuration of game=variable, ie ksp=CKANmeta

Out of Scope for this PR

It was already looking like a large set of changes, so some decisions were made to at least constrain it somewhat. Allowing to at least ensure the status quo is working before iterating to the next set of changes

• Mirrorer - The intention would be to use the generic queue implementation, bringing us to a single queue implementation. However we can't apply for a collection until we have 50 mods to upload.
• Primary branch from master to main - this seemed sensible to refactor independently
• Scheduler/Freezer etc to iterate through available games
• Common/SharedArgs naming convention - this could use improvement. Beyond the entry point, the rest of the application probably doesn't need to care about the arguments, just that they are available. With some more test coverage this could be refactored separately to this PR
• ModStatus + how we handle identifying/separating of the games

Added

• Coverage Reporting
• Game class, that returns game specific values. There is scope here for improvement, but the current implementation does work and will benefit from being used
• game-id option - this allows the existing utilities to operate as is without much change
• inflation-queues option - This is a multi var to direct inflation to the relevant queues
• BaseMessageHandler to provide a consistent interface for handling SQS messages
• QueueHandler move queue logic out of cli, and into a testable common class

Changed

• ckanmeta-remote -> ckanmeta-remotes - multi game repos ckanmeta
• netkan-remote -> netkan-remotes - multi game repos for netkan
• ia-collection -> ia-collections - allow for the configuration of multi game internet archive repos
• GitHubPR - repo changed to lazy load, avoiding it trying to auth unnecessarily on class instantiation.
• SpaceDockAdder moved to common queue/message handler
• Webhooks multi-game handling
• Webhooks queues changed to lazy load - speeds up service start and simplifies testing

Improved

• SharedArgs variables now typed at a class level and not set during init. Improves type hints during coding, and reduces likelihood of a null where it's not expected
• Boto3 type checking via mypy-boto3 - caveat of it needing if TYPE_CHECKING: to avoid upsetting pylint

Note: Merging will need to coincide with deployment of the updated configuration. I will allocate time to do this, and monitor/test relevant services after.

[HebaruSan]

SpaceDock is just about ready now (see KSP-SpaceDock/SpaceDock#480). Is there anything still missing from this, or could we merge it and fix things as they break?

[HebaruSan]

Added some comments.

In addition, a few things are unavoidable for me:

• This is a LOT of new code, and new code means new bugs (and a lot of review time)
• The existing code has been made a LOT more complex (increased complexity also means new bugs as well as harder-to-fix bugs)
• These containers simply don't need to and shouldn't know what game they're operating on; the only behavior that it determines is which repos and queues to use, which was already well handled by the previously existing structure. Everything else they do is the exact same logic regardless of what game it is, so passing around Game objects and game_id values is pure cost in terms of technical debt and cognitive load for no benefit (asterisk for webhooks).
• It's been almost three weeks and this PR isn't even out of draft status yet (not a criticism of you, but it is a reflection of the contrast between the scope of this design and our available resources)

I'm having second thoughts about closing #282 now that I've seen the details of this code I'm potentially going to have to co-maintain going forward. We probably could have wrapped that one up in another day or two of dev on the webhooks and gone live with KSP2 support weeks ago. Is there anything I can say to persuade you to consider the approach that leverages proven, stable code?

[HebaruSan]

However the queue handling [...] had three different implementations.

Yeah, that was very much intentional. Retrieving messages from a queue and deleting them is, frankly, too simple of a task to be worth centralizing. (To me it's a bit like making a wrapper around
logging.error to reduce the number of "implementations" of logging an error message; doing the thing itself instead is simpler and more maintainable.) Especially when it means that key logic that I need to read to understand what it's doing and investigate/fix bugs is now spread across multiple different files and classes rather than in a simple "X then Y then Z" procedural code block.

We're down to 2

... so naturally I'm not delighted about this. ☝️

[HebaruSan]

Added some replies to previous comments, then tried to figure out how the SpaceDockAdder works now, which led me down the rabbit hole of "when is game getting set," which was confusing because game might be a str or a Game depending on context. Added suggestions for those.

[techman83]

Yeah, that was very much intentional. Retrieving messages from a queue and deleting them is, frankly, too simple of a task to be worth centralizing. (To me it's a bit like making a wrapper around
logging.error to reduce the number of "implementations" of logging an error message; doing the thing itself instead is simpler and more maintainable.) Especially when it means that key logic that I need to read to understand what it's doing and investigate/fix bugs is now spread across multiple different files and classes rather than in a simple "X then Y then Z" procedural code block.

This decouples the queue handling, so that when debugging an issue or writing test cases, the queue can be largely ignored. They all pretty much were doing the same thing, but three different ways. I still think there is definitely room for simplifying further, but it works, has tests, and is relatively clean.

Added some replies to previous comments, then tried to figure out how the SpaceDockAdder works now, which led me down the rabbit hole of "when is game getting set," which was confusing because game might be a str or a Game depending on context. Added suggestions for those.

Yeah, valid critique. Have updated and the tests look good.

[techman83]

@HebaruSan - any other blockers here? #288 + #289 are considerably more tightly scoped, so they should be much less to get across after this one.

[techman83]

It's not really a code smell, rather a pattern that's an artifact of a "typed" runtime language. Personally I'd rather see it battle tested than spend too much time worrying about code smells. It has tests, we can refactor if we find a better way to achieve the same goal.

[techman83]

At this point I'd rather not make any further changes to the PR. Ultimately it's a pretty common python pattern, and the decision to do it this way were unrelated to any thought towards performance.

[techman83]

Ultimately it's a stylistic choice and I'm being stubborn. On reflection if that makes it easier and gets it across the line, I'm good with that.

[HebaruSan]

Let's goooo! 🏃
(I'll leave this for you to merge, so you can monitor it during deployment and rebase other PRs as needed)

[HebaruSan]

It's not really a code smell

Here's an explanation of what "code smell" means:

• https://www.youtube.com/watch?v=4gXbAday4Og

It's doesn't refer (only) to a kind of warning from a linter. It's when "part of a program is not ideally structured, not necessarily an error or a bug". Examples given are:

• long method
• long parameter list
• large class
• lazy class
• duplicated code
• message chains

"Extra code" (5+ lines to do 1 line of work) and "unmotivated lazy initialization" are in the same category of concerns.