Rewrite branding manager#1463
Conversation
Since we're planning substantial changes, it will be easier to build from scratch.
Constants will only be used in one place and there's not enough of them to warrant a separate module.
These methods form the API to the repository abstraction.
This allows us to add a neat string representation.
This adds the core logic of branding management. In comparison with the previous version, we now maintain all state in Redis, which allows the bot to seamlessly restart without losing any information. The 'send_info_embed' function is intentionally implemented with the consideration of allowing users to invoke it on-demand. It always reads information from the cache, even if the caller could pass a 'MetaFile' instance. So while this may look needlessly indirect right now, it should begin to make sense once the command API is implemented.
Sync make also be invoked with a command; avoid logic duplication.
This is a prequel to adding a calendar command. To avoid re-querying the branding repo on command invocation, event information will be cached whenever we make requests. The command can then simply get an up-to-date event schedule from the cache, with the option of forcing an update via the 'populate_cache_events' function. Since we cannot easily serialize entire 'Event' instances, we simply store what's needed - the event name, and its duration. The author has verified that the cache maintains order; in this case chronological order based on event start date.
It makes more sense for the init and the rotation to be separate operations. In a subsequent commit, the separation of responsibility will allow the `rotate_icons` function to have a meaningful return value.
The sync command will now be able to use present this information to the invoking user. This commit also prevents the cached banner & icon hash from being overwritten in the case of asset upload failure. As a result, the daemon will attempt to re-apply the assets the following day.
Now that the boolean flags are propagating from 'apply_asset', we can present them to the user.
Discord.py doesn't await the return value.
The notification is now sent conditionally depending on whether we're entering a new event. This prevents sending a repeating notification in the case of a manual resynchronisation. A practical example of when this may trigger is when a staff member temporarily applies custom assets & then uses the sync command to reapply the current event.
Knowing which event failed would probably be quite useful.
Previously, the event description & duration strings were only stored on event entry. In the case that the description or duration change for an on-going event, the cached values wouldn't be updated. After this commit, the cache is refreshed daily by the daemon.
The fetch helpers will now raise when the request fails rather than logging a warning and returning a fallback value. This allows better error logging as the caller is able to log the propagated exception while adding its own context. Additionally, the caller in some cases no longer needs to check for the None return and raise its own exception.
The default KeyError message from dict lookup is just the missing key. In order to give more context in the log message, we raise our own.
No reason for this to be async.
Logs are now proper sentences ended with full stops. Exceptions are logged with full tracebacks, and log level are revised to be more sensible and consistent across the extension.
The fallback event should not produce a notification.
It would be strange to just send the embed with no explanation of what it means.
|
Ready for review! But please point your attention here first: python-discord/branding#129. |
Conflict in the lockfile resolved by re-locking the merged Pipfile. Conflict in Branding constants resolved by keeping my local version. Change in the cog's target branch to 'main' from 'master' is currently irrelevant as we targets a development branch anyway.
There was a problem hiding this comment.
This is an unusually large PR, and it probably has to be, so I'm not going to go through every single line of code here, but I'll comment on the high level stuff.
First of all, I'd like to say that this is an exceptionally excellent pull request. The code is idiomatic, well documented, and frankly, quite beautiful. It is a real pleasure to read it.
You mention in the PR description that you haven't written any unit tests for this, and I think that's absolutely fine. We probably should have someone do a functional review of this by actually checking the code out and experimenting with it, though.
I think your approach here is extremely well thought-out and resilient. I love the use of caching to minimize the network impact, I really like the objects you've created to keep track of this, I like the use of NamedTuple classes for the metadata, and I think the suite provides all the different bot commands that will be useful to us. I generally have a hard time finding anything to complain about.
I'm approving this, and I just want to thank you for writing it. I think it'll be extremely useful to us going forward, and look forward to having it in production. 👏🏼 🏆
Shivansh-007
left a comment
Shivansh-007
left a comment
There was a problem hiding this comment.
I haven’t tested this yet. This is a pure code review. It was a lovely time reviewing this.
Like lemon said, the code is idiomatic, well documented, and frankly, quite beautiful. It is a real pleasure to read it.
<:lemon_hyperpleased:754441879822663811> (Can discord show this? hmm)
| self.available_icons = list(icons_dir.values()) | ||
| We cache `event` information to ensure that we: | ||
| * Remember which event we're currently in across restarts | ||
| * Provide an on-demand information embed without re-querying the branding repository |
There was a problem hiding this comment.
an may seem as redundant with the uncountable pronoun information here.
| * Provide an on-demand information embed without re-querying the branding repository | |
| * Provide on-demand information embed without re-querying the branding repository |
There was a problem hiding this comment.
How is 'information' a pronoun?
'... to ensure that we provide on-demand information embed' seems worse than the current revision. We're providing the embed, not the information. However, using 'an informational embed' would probably be better.
There was a problem hiding this comment.
Right agreed, "an informational embed" sounds better.
Shivansh-007
left a comment
Shivansh-007
left a comment
There was a problem hiding this comment.
Thanks! Looks Good To Me! 
jb3
left a comment
jb3
left a comment
There was a problem hiding this comment.
This all looks great and works locally!
Only minor notes, both are optional:
- The
fetch_fileandfetch_directorycould have someinfolevel logs when a request is successful. - The pending comment about the wording of the "informational embed".
Other than that, there is only a merge conflict to resolve and then we should be good!
whispersofthedawn
left a comment
whispersofthedawn
left a comment
There was a problem hiding this comment.
LGTM! And that's a lot of code. Wonderful job kwzrd!
Lockfile conflict resolved by re-locking on the merged Pipfile.
With the branding-side PR merged, we can now target the production branch.
No code changes in this commit. Co-authored-by: Shivansh-007 <Shivansh-007@users.noreply.github.com> Co-authored-by: Joe Banks <joseph@josephbanks.me>
Co-authored-by: Shivansh-007 <Shivansh-007@users.noreply.github.com> Co-authored-by: Joe Banks <joseph@josephbanks.me>
|
@Shivansh-007 Thanks for the review! @jb3 Thanks, I improved docs in 220590c and added fetch success logs in b778c25. I've chosen debug level though, hope you agree that works. Since each event requires a fetch, info would be noisy in production. @dawnofmidnight Thanks. 🤗 Solution now targets branding's production branch and should be good to go. |
|
🎉 |
|
Also thank you @lemonsaurus for the review and the kind words! It was fun to work on this. |
7 participants
Resolves #1431
Resolves #1377
This PR implements the changes proposed in #1431. The branding management extension is rewritten to comply with the new event structures in the branding repository.
What has changed?
Everything. I've completely rewritten the extension for two reasons:
So while the extension still aims to solve the same problem, it now works in a completely different way.
Implementation
I've decided to split the implementation into two classes.
1. BrandingRepository
Forms an abstraction of the remote repository that we pull events from. It allows the user to get all events represented as Python objects, validated to ensure that they are correctly defined. It also abstracts away the year-agnostic dates, and is able to select the currently active event without having to worry about dates at all.
2. Branding
This is a typical cog containing the logic needed to keep the Discord guild synchronised. It is mostly designed to be autonomous: an internal daemon will pull events every UTC midnight, transition between events when appropriate, rotate icons at the configured frequency, and even react to asset changes in on-going events. If the banner changes for an already active event, the cog will detect this by comparing its hash against the cache, and re-apply the new version. The same applies for server icons.
In order to achieve this, the cog uses 3 Redis caches. In fact, all state is stored in Redis, so there is no "warm-up" cost on restarts ~ everything should be completely smooth.
Some of the internal state is exposed via user commands: you can get information about the current event, and also see a schedule of all events.
None of this requires GitHub requests! We store everything in Redis & 1 automatic refresh a day keeps a stale cache at bay, or something.
Moderators+ can then enable & disable the daemon and force resynchronisation if necessary.
However, a manual sync is seen as more of a recovery scenario. Unless something catastrophic happens, the daemon should simply keep everything updated in the background, and require no user supervision.
A few thoughts on the testing suite
It ain't there. I've committed to reaching 100% coverage on a previous contribution, and although it allowed me to learn a lot about writing tests, in retrospect it ended up being a source of frustration, as I had to completely rewrite or remove portions of the testing suite when reacting to reviewer feedback, massively ramping up the required effort to move forward. The parts of the code that would be easy to test don't really need extensive testing, and the other parts are really difficult to test thoroughly, considering that the solution depends on two external APIs and is asynchronous. For these reasons, I decided to omit the tests this time.
How to review
When reading the code, I'd suggest starting with the repository abstraction, reading it top to bottom, and then the same for the cog. I would also recommend that reviewers read the branding-side PR!
It may be good to get that fully approved before we start thoroughly reviewing this contribution, as changes branding-side may need to be reflected here.The repository abstraction is currently targeting the source branch of the branding PR! We will update that here once the branding PR is merged.Testing
Testing may be a little intricate due to the reliance on another repository, and also due to the fact that the cog is mostly controlled by the daemon, rather than commands. When testing the daemon, it's possible to manually change the calculated sleep seconds in the before hook. Changing the loop frequency to e.g. 30 seconds will let you observe the nightly wake-ups. Similarly, you can overwrite the calculated time difference in
maybe_rotate_iconsor change the configured frequency to 0 in order to observe rotations.Since testing the detection of asset changes is a little tricky, I recorded my approach. Maybe someone will find it useful. Amongst other things, it will show you how you can easily manipulate
BrandingRepositoryto give you an event for a specific date.If you'd like to make a new branch to target in my branding fork, feel free to reach out, I will add you as a contrib.To test certain scenarios, it may also be useful to manipulate the caches using internal eval.
If you find the debug and trace logging from
BrandingRepositorynoisy, try bumping the local logger's level toINFO.I strongly recommend setting a GitHub API key when testing, as you will otherwise run into rate limits quickly!
The GitHub key can be generated in your account settings. It requires no extra permissions, as the target repo is public. It merely functions to authorize your requests and bump rate limits.
Set it in the
.envfile as such:Then, the cog will automatically use it.
Otherwise, if you're unsure on how to test something, or how something works, feel free to ask me!
Pre-merge checklist
upstream/mainwhen making GitHub requests (currently aimed at my fork)In terms of merge strategy I propose the following: