Conversation
chestercheng
left a comment
chestercheng
left a comment
There was a problem hiding this comment.
I have a couple of concerns:
- Can we add a docker-compose.yml for easier setup and testing? It would make it much easier to run and try this service.
- There are quite a lot of tests, but many look very simple or repetitive. It makes review harder, maybe we can focus on more meaningful test cases.
- Personally I prefer not to use type hints, since they add extra typing effort and don’t provide runtime checks.
| ) | ||
|
|
||
| return { | ||
| "content": f"📊 **Argus Daily Registration Summary** {now_str} (Asia/Taipei)", |
There was a problem hiding this comment.
I think we don't need to include now_str in the message content, Since Discord already shows a timestamp for each message.
| "content": f"📊 **Argus Daily Registration Summary** {now_str} (Asia/Taipei)", | |
| "content": f"📊 **Argus Daily Registration Summary**", |
| # JSON-LD startDate field | ||
| _JSONLD_START_RE = re.compile(r'"startDate"\s*:\s*"([^"]+)"') | ||
| # fa-male icon followed by "current / total" | ||
| _CAPACITY_RE = re.compile(r'fa-male"></i>\s*\d+\s*/\s*(\d+)') |
There was a problem hiding this comment.
Suggest using TextFSM instead of regex here. It is easier to read and maintain.
| @@ -0,0 +1 @@ | |||
| __version__ = "0.1.0" | |||
There was a problem hiding this comment.
Do we need a separate __about__.py?
Can we just put this in __init__.py to keep things simpler?
| def is_email_allowed(email: str) -> bool: | ||
| if not email: | ||
| return False | ||
| return email.lower() in {e.lower() for e in config.settings.allowed_emails} |
There was a problem hiding this comment.
I'm not sure if we should keep the allowlist inside the app.
If we update it, we need to restart the service.
| return cls( | ||
| webhook_secret=os.getenv("WEBHOOK_SECRET", ""), | ||
| google_oauth_client_id=os.getenv("GOOGLE_OAUTH_CLIENT_ID", ""), | ||
| google_oauth_client_secret=os.getenv("GOOGLE_OAUTH_CLIENT_SECRET", ""), |
There was a problem hiding this comment.
Do we really want to require OAuth for all environments?
This makes local development hard. Can we make it optional?
There was a problem hiding this comment.
Provide 2 env example, 1 for development (disable oauth), one for production (MUST enable oauth).
|
|
||
| RUN pip install --no-cache-dir . | ||
|
|
||
| CMD ["argus"] |
There was a problem hiding this comment.
Using uvicorn argus.main:app in Docker might give us more flexibility (e.g. configuring workers, reload, etc).
| "httpx", | ||
| ] | ||
|
|
||
| [build-system] |
There was a problem hiding this comment.
Do we really need to package this as a Python project?
This is a service not a library. Maybe we can keep pyproject.toml simpler.
| @@ -0,0 +1,9 @@ | |||
| { | |||
There was a problem hiding this comment.
I’m not familiar with railway.json. What is this used for in this project?
| @@ -0,0 +1,8 @@ | |||
| fastapi | |||
There was a problem hiding this comment.
Since we already use pyproject.toml, I think we can remove requirements.txt to avoid duplication.
| @@ -0,0 +1 @@ | |||
|
|
|||
There was a problem hiding this comment.
This is empty, do you plan to add a license, or should we remove it?
2 participants
Build initial Argus webhook service with dashboard