Conversation
egeakman
left a comment
egeakman
left a comment
There was a problem hiding this comment.
Other than the previous comment, nothing catches my eye. I didn't go through the code line by line though.
| else: | ||
| warnings.warn(f"{name} not set") | ||
|
|
||
| return value or "" |
There was a problem hiding this comment.
This is a bot hard for me to understand. Can we make it a bit easier for novices to get the logic?
There was a problem hiding this comment.
I'll think about this. I'll leave it as-is for now, but there is probably a better way of doing it.
The gist is that on prod we want to force all of those, in other environments they are optional.
I totally agree that there must be a better way to do it.
There was a problem hiding this comment.
Crazy idea:
(Data)classes with inheritance?
There was a problem hiding this comment.
For environments, or for individual settings?
| DISCORD_EP2025_CHANNEL_ID = get("DISCORD_EP2025_CHANNEL_ID") | ||
| DISCORD_EP2025_CHANNEL_NAME = get("DISCORD_EP2025_CHANNEL_NAME") |
There was a problem hiding this comment.
Could we please not fix this on 2025? But in a general "organisers" channel or "current conference" channel?
There was a problem hiding this comment.
I would like to keep it explicit :) I would actually think those are mostly constant and won't change, so in the future instead of changing settings for current/future conf, we just update the code and have another 1:1 mapping from project ID to channel ID. (and optionally delete old ones or just keep them around unused)
There was a problem hiding this comment.
The more I think about it the less I like the hardcoded part.
It should be some kind of extracted config structure that allows the allocation.
{ "channel_type" : {"name" : "", "discord_id" : 000, "github_id" : [00, 01]}}
(And I am pretty sure that the order in my example is the wrong one.)
| GITHUB_EP2025_PROJECT_ID = get("GITHUB_EP2025_PROJECT_ID") | ||
| GITHUB_EM_PROJECT_ID = get("GITHUB_EM_PROJECT_ID") | ||
|
|
||
| if DJANGO_ENV == "dev": |
There was a problem hiding this comment.
Why is there no DJANGO_ENV prod needed?
There was a problem hiding this comment.
So... it is – as in, if we want to deploy on prod they need to be provided. If you want to run tests they are still needed - but overwritten below in the if DJANGO_ENV == test section.
There was a problem hiding this comment.
As you see — makes it hard for me to understand.
There should probably some kind of check around here that tells me that I forgot to inject something in PROD.
There was a problem hiding this comment.
There is a check – on prod it will just not start unless you provide everything. On dev it will start but emit a warning in the console.
There was a problem hiding this comment.
Rereading the original comment now – I think there might be some confusion that there is no setup for prod – there is one, but it already existed before so it's outside of this diff. :)
Co-authored-by: Cyril Bitterich <cebit-github@gunnet.de>
| else: | ||
| warnings.warn(f"{name} not set") | ||
|
|
||
| return value or "" |
There was a problem hiding this comment.
Crazy idea:
(Data)classes with inheritance?
| DISCORD_EP2025_CHANNEL_ID = get("DISCORD_EP2025_CHANNEL_ID") | ||
| DISCORD_EP2025_CHANNEL_NAME = get("DISCORD_EP2025_CHANNEL_NAME") |
There was a problem hiding this comment.
The more I think about it the less I like the hardcoded part.
It should be some kind of extracted config structure that allows the allocation.
{ "channel_type" : {"name" : "", "discord_id" : 000, "github_id" : [00, 01]}}
(And I am pretty sure that the order in my example is the wrong one.)
| GITHUB_EP2025_PROJECT_ID = get("GITHUB_EP2025_PROJECT_ID") | ||
| GITHUB_EM_PROJECT_ID = get("GITHUB_EM_PROJECT_ID") | ||
|
|
||
| if DJANGO_ENV == "dev": |
There was a problem hiding this comment.
As you see — makes it hard for me to understand.
There should probably some kind of check around here that tells me that I forgot to inject something in PROD.
4 participants
Initial version of routing discord messages to different channels depending on the details of the github webhook.
Designed to be extendible with different types/sources of webhooks.
It's still a little bit WiP, but the tests are passing.