-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Game Management #283
Conversation
8847659
to
c030f45
Compare
ce9874f
to
d04292d
Compare
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 andgame_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?
This is a working example of how we could support multiple games
Updated the variables to more accurately reflect they contain multiple vars
51471cc
to
54a9591
Compare
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
... so naturally I'm not delighted about this. ☝️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This updates the game to game_id, making it more consistent throughout the codebase Co-authored-by: HebaruSan <HebaruSan@users.noreply.github.com>
0a43485
to
8c75711
Compare
This moves the queue handling out of common into a more relevantly named section of the codebase.
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.
Yeah, valid critique. Have updated and the tests look good. |
@HebaruSan - any other blockers here? #288 + #289 are considerably more tightly scoped, so they should be much less to get across after this one. |
This comment was marked as resolved.
This comment was marked as resolved.
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. |
This comment was marked as resolved.
This comment was marked as resolved.
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. |
This comment was marked as outdated.
This comment was marked as outdated.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's goooo! 🏃
(I'll leave this for you to merge, so you can monitor it during deployment and rebase other PRs as needed)
Here's an explanation of what "code smell" means: 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:
"Extra code" (5+ lines to do 1 line of work) and "unmotivated lazy initialization" are in the same category of concerns. |
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 varOPTION=var var
. This allows a configuration ofgame=variable
, ieksp=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
master
tomain
- this seemed sensible to refactor independentlyAdded
Game
class, that returns game specific values. There is scope here for improvement, but the current implementation does work and will benefit from being usedBaseMessageHandler
to provide a consistent interface for handling SQS messagesQueueHandler
move queue logic out of cli, and into a testable common classChanged
GitHubPR
- repo changed to lazy load, avoiding it trying to auth unnecessarily on class instantiation.SpaceDockAdder
moved to common queue/message handlerImproved
if TYPE_CHECKING:
to avoid upsetting pylintNote: Merging will need to coincide with deployment of the updated configuration. I will allocate time to do this, and monitor/test relevant services after.