Created
April 1, 2026 19:29
-
-
Save eric239/e1d686267788aefa5454216b61df5173 to your computer and use it in GitHub Desktop.
PR #4633 agent review report
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| <!DOCTYPE html> | |
| <html lang="en"> | |
| <head> | |
| <meta charset="UTF-8"> | |
| <meta name="viewport" content="width=device-width, initial-scale=1.0"> | |
| <title>PR #4633 Review - Running service under uvicorn</title> | |
| <style> | |
| :root { | |
| --bg: #0d1117; | |
| --surface: #161b22; | |
| --border: #30363d; | |
| --text: #e6edf3; | |
| --text-muted: #8b949e; | |
| --accent: #58a6ff; | |
| --green: #3fb950; | |
| --red: #f85149; | |
| --green-bg: #12261e; | |
| --red-bg: #2d1214; | |
| --yellow: #d29922; | |
| --purple: #bc8cff; | |
| --orange: #f0883e; | |
| --syn-keyword: #ff7b72; | |
| --syn-string: #a5d6ff; | |
| --syn-decorator: #d2a8ff; | |
| --syn-func: #d2a8ff; | |
| --syn-class: #ffa657; | |
| --syn-builtin: #ffa657; | |
| --syn-comment: #8b949e; | |
| --syn-self: #ff7b72; | |
| --syn-number: #79c0ff; | |
| --syn-type: #ffa657; | |
| } | |
| * { margin: 0; padding: 0; box-sizing: border-box; } | |
| body { background: var(--bg); color: var(--text); font-family: -apple-system, BlinkMacSystemFont, 'Segoe UI', Helvetica, Arial, sans-serif; line-height: 1.6; padding: 2rem; max-width: 1200px; margin: 0 auto; } | |
| a { color: var(--accent); text-decoration: none; } | |
| a:hover { text-decoration: underline; } | |
| .header { border-bottom: 1px solid var(--border); padding-bottom: 1.5rem; margin-bottom: 1.5rem; } | |
| .header h1 { font-size: 1.5rem; font-weight: 600; margin-bottom: 0.5rem; } | |
| .header h1 .pr-num { color: var(--text-muted); font-weight: 400; } | |
| .header-meta { display: flex; gap: 1.5rem; color: var(--text-muted); font-size: 0.85rem; flex-wrap: wrap; } | |
| .header-meta span { display: flex; align-items: center; gap: 0.3rem; } | |
| .stat-add { color: var(--green); } | |
| .stat-del { color: var(--red); } | |
| .tldr { border-left: 3px solid var(--accent); background: rgba(88,166,255,0.08); padding: 1rem 1.2rem; margin-bottom: 2rem; border-radius: 0 6px 6px 0; } | |
| .tldr p { font-size: 0.95rem; } | |
| .card { background: var(--surface); border: 1px solid var(--border); border-radius: 8px; margin-bottom: 1.5rem; overflow: hidden; } | |
| .card-header { padding: 1rem 1.2rem; border-bottom: 1px solid var(--border); display: flex; align-items: center; gap: 0.75rem; } | |
| .card-header .num { background: var(--accent); color: var(--bg); width: 28px; height: 28px; border-radius: 50%; display: flex; align-items: center; justify-content: center; font-size: 0.8rem; font-weight: 700; flex-shrink: 0; } | |
| .card-header h2 { font-size: 1.1rem; font-weight: 600; } | |
| .card-body { display: flex; } | |
| .card-files { width: 220px; background: rgba(13,17,23,0.5); padding: 1rem; border-right: 1px solid var(--border); flex-shrink: 0; } | |
| .card-files ul { list-style: none; font-size: 0.78rem; font-family: 'SFMono-Regular', Consolas, monospace; } | |
| .card-files li { padding: 0.2rem 0; display: flex; align-items: center; gap: 0.4rem; word-break: break-all; } | |
| .card-files .dot { width: 8px; height: 8px; border-radius: 50%; flex-shrink: 0; } | |
| .dot-added { background: var(--green); } | |
| .dot-modified { background: var(--yellow); } | |
| .dot-removed { background: var(--red); } | |
| .card-desc { padding: 1rem 1.2rem; flex: 1; min-width: 0; } | |
| .card-desc h3 { font-size: 0.85rem; color: var(--text-muted); text-transform: uppercase; letter-spacing: 0.05em; margin-bottom: 0.5rem; } | |
| .card-desc p { font-size: 0.9rem; margin-bottom: 0.75rem; } | |
| .diffs { border-top: 1px solid var(--border); padding: 1rem 1.2rem; } | |
| .diffs h3 { font-size: 0.85rem; color: var(--text-muted); text-transform: uppercase; letter-spacing: 0.05em; margin-bottom: 0.75rem; } | |
| .diff-file { margin-bottom: 1rem; } | |
| .diff-file-name { font-size: 0.78rem; font-family: 'SFMono-Regular', Consolas, monospace; color: var(--text-muted); margin-bottom: 0.25rem; } | |
| pre.diff-block { background: var(--bg); border: 1px solid var(--border); border-radius: 6px; padding: 0.5rem 0; overflow-x: auto; font-size: 0; line-height: 0; } | |
| .diff-line { display: block; padding: 1px 1rem; font-family: 'SFMono-Regular', Consolas, monospace; font-size: 0.72rem; line-height: 1.45; white-space: pre; } | |
| .diff-line.add { background: var(--green-bg); } | |
| .diff-line.remove { background: var(--red-bg); } | |
| .diff-line.context { color: var(--text-muted); } | |
| .diff-line.ellipsis { text-align: center; color: var(--text-muted); font-style: italic; } | |
| .diff-line .kw { color: var(--syn-keyword); } | |
| .diff-line .str { color: var(--syn-string); } | |
| .diff-line .dec { color: var(--syn-decorator); } | |
| .diff-line .fn { color: var(--syn-func); } | |
| .diff-line .cls { color: var(--syn-class); } | |
| .diff-line .ty { color: var(--syn-type); } | |
| .diff-line .bi { color: var(--syn-builtin); } | |
| .diff-line .num { color: var(--syn-number); background: none; width: auto; height: auto; border-radius: 0; display: inline; font-weight: normal; font-size: inherit; } | |
| .diff-line .slf { color: var(--syn-self); font-style: italic; } | |
| .diff-line .cmt { color: var(--syn-comment); font-style: italic; } | |
| .concerns { border-top: 1px solid var(--border); padding: 1rem 1.2rem; } | |
| .concerns h3 { font-size: 0.85rem; color: var(--text-muted); text-transform: uppercase; letter-spacing: 0.05em; margin-bottom: 0.75rem; } | |
| .concern-box { padding: 0.75rem 1rem; border-radius: 6px; margin-bottom: 0.5rem; font-size: 0.88rem; } | |
| .concern-box.issue { background: rgba(210,153,34,0.1); border-left: 3px solid var(--yellow); } | |
| .concern-box.ok { background: rgba(63,185,80,0.08); border-left: 3px solid var(--green); } | |
| .concern-box strong { color: var(--yellow); } | |
| .summary-table { margin-top: 2rem; } | |
| .summary-table h2 { font-size: 1.2rem; margin-bottom: 1rem; } | |
| table { width: 100%; border-collapse: collapse; background: var(--surface); border: 1px solid var(--border); border-radius: 8px; overflow: hidden; } | |
| th { background: rgba(13,17,23,0.5); text-align: left; padding: 0.7rem 1rem; font-size: 0.8rem; color: var(--text-muted); text-transform: uppercase; letter-spacing: 0.05em; border-bottom: 1px solid var(--border); } | |
| td { padding: 0.6rem 1rem; border-bottom: 1px solid var(--border); font-size: 0.88rem; } | |
| tr:last-child td { border-bottom: none; } | |
| .severity { display: inline-block; padding: 0.15rem 0.5rem; border-radius: 10px; font-size: 0.75rem; font-weight: 600; } | |
| .severity.bug { background: var(--red-bg); color: var(--red); } | |
| .severity.design { background: rgba(240,136,62,0.15); color: var(--orange); } | |
| .severity.minor { background: rgba(210,153,34,0.15); color: var(--yellow); } | |
| .severity.question { background: rgba(188,140,255,0.15); color: var(--purple); } | |
| .severity.nit { background: rgba(139,148,158,0.15); color: var(--text-muted); } | |
| .severity.expected { background: rgba(88,166,255,0.1); color: var(--accent); } | |
| </style> | |
| </head> | |
| <body> | |
| <div class="header"> | |
| <h1>Running service under uvicorn <span class="pr-num">#4633</span></h1> | |
| <div class="header-meta"> | |
| <span>Author: <strong>eric239</strong></span> | |
| <span>Branch: <strong>tech-6304-prepare-email-service-scaffolding-3</strong></span> | |
| <span>Files: <strong>17</strong></span> | |
| <span><span class="stat-add">+544</span> / <span class="stat-del">-124</span></span> | |
| </div> | |
| </div> | |
| <div class="tldr"> | |
| <p>This PR wires up the vendor-comms service to run under uvicorn with graceful k8s shutdown support, and extracts shared admin, middleware, and sentry utilities from the main <code>api</code> project into <code>shared-utils</code> and <code>shared-config</code> libraries for cross-service reuse.</p> | |
| </div> | |
| <!-- Architecture Diagram --> | |
| <div style="margin-bottom: 2rem; background: var(--surface); border: 1px solid var(--border); border-radius: 8px; padding: 1.5rem;"> | |
| <svg viewBox="0 0 1000 420" xmlns="http://www.w3.org/2000/svg" style="width:100%;height:auto"> | |
| <defs> | |
| <marker id="arrow" viewBox="0 0 10 7" refX="10" refY="3.5" markerWidth="10" markerHeight="7" markerUnits="userSpaceOnUse" orient="auto"> | |
| <path d="M 0 0 L 10 3.5 L 0 7 z" fill="#8b949e"/> | |
| </marker> | |
| </defs> | |
| <!-- Title --> | |
| <text x="500" y="30" fill="#e6edf3" font-size="14" font-weight="600" text-anchor="middle">Service Startup & Shared Library Architecture</text> | |
| <!-- uvicorn.sh (new) --> | |
| <rect x="40" y="60" width="180" height="60" rx="8" fill="none" stroke="#3fb950" stroke-width="2" onclick="scrollToCard('block-1')" style="cursor:pointer"/> | |
| <text x="130" y="85" fill="#3fb950" font-size="11" font-weight="600" text-anchor="middle">services/uvicorn.sh</text> | |
| <text x="130" y="103" fill="#8b949e" font-size="10" text-anchor="middle">SIGTERM trap + marker</text> | |
| <!-- vendor-comms settings (modified) --> | |
| <rect x="280" y="60" width="200" height="60" rx="8" fill="none" stroke="#d29922" stroke-width="2" onclick="scrollToCard('block-4')" style="cursor:pointer"/> | |
| <text x="380" y="85" fill="#d29922" font-size="11" font-weight="600" text-anchor="middle">vendor_comms/settings.py</text> | |
| <text x="380" y="103" fill="#8b949e" font-size="10" text-anchor="middle">Django + Celery + CORS</text> | |
| <!-- Arrow: uvicorn.sh -> settings --> | |
| <line x1="220" y1="90" x2="275" y2="90" stroke="#8b949e" stroke-width="1.5" marker-end="url(#arrow)"/> | |
| <text x="248" y="83" fill="#8b949e" font-size="9" text-anchor="middle">launches</text> | |
| <!-- shared-config (modified) --> | |
| <rect x="180" y="190" width="200" height="60" rx="8" fill="none" stroke="#d29922" stroke-width="2" onclick="scrollToCard('block-2')" style="cursor:pointer"/> | |
| <text x="280" y="215" fill="#d29922" font-size="11" font-weight="600" text-anchor="middle">shared-config</text> | |
| <text x="280" y="233" fill="#8b949e" font-size="10" text-anchor="middle">BaseServiceSettings, setup_sentry</text> | |
| <!-- shared-utils (modified) --> | |
| <rect x="440" y="190" width="200" height="60" rx="8" fill="none" stroke="#d29922" stroke-width="2" onclick="scrollToCard('block-3')" style="cursor:pointer"/> | |
| <text x="540" y="215" fill="#d29922" font-size="11" font-weight="600" text-anchor="middle">shared-utils</text> | |
| <text x="540" y="233" fill="#8b949e" font-size="10" text-anchor="middle">admin, middleware, db_utils</text> | |
| <!-- Arrow: settings -> shared-config --> | |
| <line x1="340" y1="120" x2="300" y2="185" stroke="#8b949e" stroke-width="1.5" marker-end="url(#arrow)"/> | |
| <text x="305" y="150" fill="#8b949e" font-size="9" text-anchor="middle">imports</text> | |
| <!-- Arrow: settings -> shared-utils --> | |
| <line x1="420" y1="120" x2="510" y2="185" stroke="#8b949e" stroke-width="1.5" marker-end="url(#arrow)"/> | |
| <text x="475" y="150" fill="#8b949e" font-size="9" text-anchor="middle">imports</text> | |
| <!-- api project (unchanged) --> | |
| <rect x="700" y="60" width="180" height="60" rx="8" fill="none" stroke="#58a6ff" stroke-width="2" stroke-dasharray="4"/> | |
| <text x="790" y="85" fill="#58a6ff" font-size="11" font-weight="600" text-anchor="middle">api/ (main project)</text> | |
| <text x="790" y="103" fill="#8b949e" font-size="10" text-anchor="middle">still has own copies</text> | |
| <!-- Arrow: api -> shared-utils (dashed, planned) --> | |
| <line x1="700" y1="110" x2="590" y2="185" stroke="#8b949e" stroke-width="1" stroke-dasharray="4" marker-end="url(#arrow)"/> | |
| <text x="660" y="152" fill="#8b949e" font-size="9" text-anchor="middle">follow-up</text> | |
| <!-- pyproject.toml (modified) --> | |
| <rect x="280" y="320" width="200" height="60" rx="8" fill="none" stroke="#d29922" stroke-width="2" onclick="scrollToCard('block-5')" style="cursor:pointer"/> | |
| <text x="380" y="345" fill="#d29922" font-size="11" font-weight="600" text-anchor="middle">pyproject.toml</text> | |
| <text x="380" y="363" fill="#8b949e" font-size="10" text-anchor="middle">constraint-dependencies</text> | |
| <!-- Arrow: shared-config -> pyproject --> | |
| <line x1="280" y1="250" x2="340" y2="315" stroke="#8b949e" stroke-width="1.5" marker-end="url(#arrow)"/> | |
| <!-- Arrow: shared-utils -> pyproject --> | |
| <line x1="500" y1="250" x2="430" y2="315" stroke="#8b949e" stroke-width="1.5" marker-end="url(#arrow)"/> | |
| <!-- Legend --> | |
| <rect x="720" y="320" width="12" height="12" rx="2" fill="none" stroke="#3fb950" stroke-width="2"/> | |
| <text x="740" y="331" fill="#8b949e" font-size="10">New</text> | |
| <rect x="720" y="342" width="12" height="12" rx="2" fill="none" stroke="#d29922" stroke-width="2"/> | |
| <text x="740" y="353" fill="#8b949e" font-size="10">Modified</text> | |
| <rect x="720" y="364" width="12" height="12" rx="2" fill="none" stroke="#58a6ff" stroke-width="2" stroke-dasharray="4"/> | |
| <text x="740" y="375" fill="#8b949e" font-size="10">Unchanged (context)</text> | |
| </svg> | |
| </div> | |
| <!-- Block 1 --> | |
| <div class="card" id="block-1"> | |
| <div class="card-header"> | |
| <div class="num">1</div> | |
| <h2>Uvicorn Startup Script</h2> | |
| </div> | |
| <div class="card-body"> | |
| <div class="card-files"> | |
| <ul> | |
| <li><span class="dot dot-added"></span>services/uvicorn.sh</li> | |
| <li><span class="dot dot-modified"></span>services/vendor-comms/Makefile</li> | |
| </ul> | |
| </div> | |
| <div class="card-desc"> | |
| <h3>What it does</h3> | |
| <p>A generic bash entrypoint that launches uvicorn in the background, sets up a SIGTERM trap with configurable delay for k8s graceful shutdown. On SIGTERM, it creates a marker file (so liveness probes return 503), waits for endpoint propagation, then forwards SIGTERM to uvicorn. The timeline fits within k8s' 15s <code>terminationGracePeriodSeconds</code>.</p> | |
| <h3>Design Decisions</h3> | |
| <p>Uses a background process + trap pattern instead of relying on uvicorn's built-in signal handling. This lets the script insert a delay between receiving SIGTERM and forwarding it, giving k8s time to propagate endpoint removal. The shutdown marker file is a lightweight mechanism for the liveness middleware to detect shutdown state.</p> | |
| </div> | |
| </div> | |
| <div class="diffs"> | |
| <h3>Key Diffs</h3> | |
| <div class="diff-file"> | |
| <div class="diff-file-name">services/uvicorn.sh (shutdown handler + wait)</div> | |
| <pre class="diff-block"><span class="diff-line add">+<span class="fn">_shutdown</span>() {</span> | |
| <span class="diff-line add">+ <span class="bi">echo</span> <span class="str">"[shutdown] SIGTERM received, creating marker and delaying ${shutdown_delay}s for nginx"</span></span> | |
| <span class="diff-line add">+ touch <span class="str">"$SHUTDOWN_MARKER"</span></span> | |
| <span class="diff-line add">+ sleep <span class="str">"$shutdown_delay"</span></span> | |
| <span class="diff-line add">+ <span class="bi">echo</span> <span class="str">"[shutdown] Forwarding SIGTERM to uvicorn (PID ${UVICORN_PID})"</span></span> | |
| <span class="diff-line add">+ kill -TERM <span class="str">"$UVICORN_PID"</span> <span class="num">2</span>>/dev/null || <span class="kw">true</span></span> | |
| <span class="diff-line add">+}</span> | |
| <span class="diff-line ellipsis">...</span> | |
| <span class="diff-line add">+trap _shutdown TERM</span> | |
| <span class="diff-line add">+trap <span class="str">'rm -f "$SHUTDOWN_MARKER"'</span> EXIT</span> | |
| <span class="diff-line add">+</span> | |
| <span class="diff-line add">+wait $UVICORN_PID</span></pre> | |
| </div> | |
| </div> | |
| <div class="concerns"> | |
| <h3>Concerns</h3> | |
| <div class="concern-box issue"><strong>#1 Bug:</strong> <code>wait $UVICORN_PID</code> does not re-wait after trap handler. When bash receives SIGTERM during <code>wait</code>, it runs the trap handler and returns immediately with the trap's exit status, not the child's. The script exits before uvicorn finishes draining, and the exit code is not propagated to k8s.</div> | |
| <div class="concern-box issue"><strong>#2 Minor:</strong> <code>$DDTRACE</code> unquoted and possibly unset. If empty (local dev without datadog), behavior is ambiguous. Use <code>${DDTRACE:-}</code> or a conditional.</div> | |
| <div class="concern-box issue"><strong>#3 Minor (1/2):</strong> Race window between uvicorn backgrounding (line 78) and trap registration (line 90). SIGTERM in this window causes default immediate termination.</div> | |
| </div> | |
| </div> | |
| <!-- Block 2 --> | |
| <div class="card" id="block-2"> | |
| <div class="card-header"> | |
| <div class="num">2</div> | |
| <h2>Shared Config: Sentry, CORS, Settings Base</h2> | |
| </div> | |
| <div class="card-body"> | |
| <div class="card-files"> | |
| <ul> | |
| <li><span class="dot dot-modified"></span>shared_config/base.py</li> | |
| <li><span class="dot dot-added"></span>shared_config/setup_sentry.py</li> | |
| <li><span class="dot dot-modified"></span>shared-config/pyproject.toml</li> | |
| </ul> | |
| </div> | |
| <div class="card-desc"> | |
| <h3>What it does</h3> | |
| <p>Extends <code>BaseServiceSettings</code> with sentry, CORS, and hostname fields. Creates a new <code>setup_sentry</code> module that consolidates sentry initialization into a reusable function with toggles for celery and pymongo integrations. CORS config generates environment-aware origins based on hostname.</p> | |
| <h3>Design Decisions</h3> | |
| <p><code>hostname</code> is a <code>ClassVar[str]</code> because it's a per-service constant, not an env var. <code>cors_origins</code> returns a 3-tuple matching the Django settings destructuring pattern. <code>is_sentry_enabled</code> fails fast with <code>ValueError</code> if DSN is missing in cloud environments.</p> | |
| </div> | |
| </div> | |
| <div class="diffs"> | |
| <h3>Key Diffs</h3> | |
| <div class="diff-file"> | |
| <div class="diff-file-name">libs/shared-config/shared_config/base.py</div> | |
| <pre class="diff-block"><span class="diff-line add">+ sentry_dsn: <span class="ty">str</span> = pyd.<span class="cls">Field</span>(alias=<span class="str">'SENTRY_DSN'</span>, default=<span class="str">''</span>)</span> | |
| <span class="diff-line add">+ is_sentry_forced_on: <span class="ty">bool</span> = pyd.<span class="cls">Field</span>(alias=<span class="str">'SENTRY_FORCED_ON'</span>, default=<span class="kw">False</span>)</span> | |
| <span class="diff-line add">+ hostname: <span class="ty">ClassVar</span>[<span class="ty">str</span>] = <span class="str">''</span></span> | |
| <span class="diff-line ellipsis">...</span> | |
| <span class="diff-line add">+ @<span class="dec">cached_property</span></span> | |
| <span class="diff-line add">+ <span class="kw">def</span> <span class="fn">cors_origins</span>(<span class="slf">self</span>) -> <span class="ty">tuple</span>[<span class="ty">bool</span>, <span class="ty">list</span>[<span class="ty">str</span>], <span class="ty">list</span>[<span class="ty">str</span>]]:</span> | |
| <span class="diff-line add">+ <span class="kw">if</span> <span class="kw">not</span> <span class="slf">self</span>.is_cloud:</span> | |
| <span class="diff-line add">+ <span class="kw">return</span> <span class="kw">True</span>, [], []</span> | |
| <span class="diff-line ellipsis">...</span> | |
| <span class="diff-line add">+ @<span class="dec">cached_property</span></span> | |
| <span class="diff-line add">+ <span class="kw">def</span> <span class="fn">is_sentry_enabled</span>(<span class="slf">self</span>) -> <span class="ty">bool</span>:</span> | |
| <span class="diff-line add">+ result = <span class="slf">self</span>.is_sentry_forced_on <span class="kw">or</span> <span class="slf">self</span>.is_cloud</span> | |
| <span class="diff-line add">+ <span class="kw">if</span> result <span class="kw">and</span> <span class="kw">not</span> <span class="slf">self</span>.sentry_dsn:</span> | |
| <span class="diff-line add">+ <span class="kw">raise</span> <span class="cls">ValueError</span>(<span class="str">'SENTRY_DSN is not set'</span>)</span> | |
| <span class="diff-line add">+ <span class="kw">return</span> result</span></pre> | |
| </div> | |
| <div class="diff-file"> | |
| <div class="diff-file-name">libs/shared-config/shared_config/setup_sentry.py</div> | |
| <pre class="diff-block"><span class="diff-line add">+<span class="kw">def</span> <span class="fn">setup_sentry</span>(</span> | |
| <span class="diff-line add">+ settings: <span class="ty">BaseServiceSettings</span>,</span> | |
| <span class="diff-line add">+ *,</span> | |
| <span class="diff-line add">+ is_celery_enabled: <span class="ty">bool</span> = <span class="kw">True</span>,</span> | |
| <span class="diff-line add">+ is_pymongo_enabled: <span class="ty">bool</span> = <span class="kw">True</span>,</span> | |
| <span class="diff-line add">+ ignored_exceptions: <span class="ty">tuple</span>[<span class="ty">type</span>, ...] = (),</span> | |
| <span class="diff-line add">+) -> <span class="kw">None</span>:</span></pre> | |
| </div> | |
| </div> | |
| <div class="concerns"> | |
| <h3>Concerns</h3> | |
| <div class="concern-box issue"><strong>#4 Minor:</strong> <code>sentry-sdk[pymongo]</code> as a dependency of <code>shared-config</code> forces pymongo sentry extras on all consumers, even services that don't use MongoDB.</div> | |
| </div> | |
| </div> | |
| <!-- Block 3 --> | |
| <div class="card" id="block-3"> | |
| <div class="card-header"> | |
| <div class="num">3</div> | |
| <h2>Shared Utils: Admin, Middleware, DB Utils</h2> | |
| </div> | |
| <div class="card-body"> | |
| <div class="card-files"> | |
| <ul> | |
| <li><span class="dot dot-added"></span>shared_utils/admin.py</li> | |
| <li><span class="dot dot-added"></span>shared_utils/middleware.py</li> | |
| <li><span class="dot dot-modified"></span>shared_utils/db_utils.py</li> | |
| <li><span class="dot dot-modified"></span>shared_utils/__init__.py</li> | |
| <li><span class="dot dot-modified"></span>shared-utils/pyproject.toml</li> | |
| </ul> | |
| </div> | |
| <div class="card-desc"> | |
| <h3>What it does</h3> | |
| <p>Adds admin utilities (<code>CustomPGHistoryEventsAdmin</code>, <code>SoftDeleteAdminMixin</code>, <code>ActiveInactiveFilter</code>, <code>ReadOnlyInline</code>, <code>DataClassFormField</code>) and middleware (<code>LivenessMiddleware</code>, <code>CustomHistoryMiddleware</code>) to shared-utils. <code>LivenessMiddleware</code> is a simplified version without gevent timeout handling. Also adds <code>TModel</code> TypeVar to <code>db_utils.py</code>.</p> | |
| <h3>Design Decisions</h3> | |
| <p>These classes are needed by vendor-comms but previously only existed in the <code>api</code> project. Rather than making vendor-comms depend on <code>api</code>, they're extracted into shared-utils. The api project's copies still exist side by side, presumably to be migrated in a follow-up.</p> | |
| </div> | |
| </div> | |
| <div class="diffs"> | |
| <h3>Key Diffs</h3> | |
| <div class="diff-file"> | |
| <div class="diff-file-name">libs/shared-utils/shared_utils/__init__.py</div> | |
| <pre class="diff-block"><span class="diff-line add">+<span class="kw">import</span> django_stubs_ext</span> | |
| <span class="diff-line add">+</span> | |
| <span class="diff-line add">+django_stubs_ext.monkeypatch()</span></pre> | |
| </div> | |
| <div class="diff-file"> | |
| <div class="diff-file-name">libs/shared-utils/shared_utils/middleware.py (logger inconsistency)</div> | |
| <pre class="diff-block"><span class="diff-line add">+<span class="kw">import</span> structlog</span> | |
| <span class="diff-line ellipsis">...</span> | |
| <span class="diff-line add">+_logger = structlog.<span class="fn">getLogger</span>(__name__) <span class="cmt"># should be get_logger</span></span></pre> | |
| </div> | |
| </div> | |
| <div class="concerns"> | |
| <h3>Concerns</h3> | |
| <div class="concern-box issue"><strong>#5 Design:</strong> <code>django_stubs_ext.monkeypatch()</code> in <code>shared_utils/__init__.py</code> runs at import time in production. The main api project calls <code>monkeypatch(extra_classes=[generics.GenericAPIView])</code> in <code>api/__init__.py</code>. Since shared_utils is imported first, the no-arg call fires first, making the GenericAPIView patching depend on import order. Better to let each project call monkeypatch explicitly.</div> | |
| <div class="concern-box issue"><strong>#6 Design (1/2):</strong> <code>TModel</code> TypeVar defined in both <code>api/types.py</code> and <code>shared_utils/db_utils.py</code>. Two separate TypeVars with the same name are not interchangeable from a type-checker perspective. Mixing imports from both will cause mypy errors.</div> | |
| <div class="concern-box issue"><strong>#7 Nit (1/2):</strong> <code>structlog.getLogger</code> in <code>shared_utils/middleware.py</code>; the rest of the codebase uses <code>structlog.get_logger</code>.</div> | |
| <div class="concern-box issue"><strong>#8 Nit (1/2):</strong> Unused <code>_logger</code> in <code>shared_utils/admin.py</code> (declared at line 15, never referenced).</div> | |
| </div> | |
| </div> | |
| <!-- Block 4 --> | |
| <div class="card" id="block-4"> | |
| <div class="card-header"> | |
| <div class="num">4</div> | |
| <h2>Vendor-Comms Settings & Configuration</h2> | |
| </div> | |
| <div class="card-body"> | |
| <div class="card-files"> | |
| <ul> | |
| <li><span class="dot dot-modified"></span>vendor_comms/settings.py</li> | |
| <li><span class="dot dot-modified"></span>vendor-comms/pyproject.toml</li> | |
| <li><span class="dot dot-modified"></span>vendor-comms/Makefile</li> | |
| </ul> | |
| </div> | |
| <div class="card-desc"> | |
| <h3>What it does</h3> | |
| <p>Fully fleshed out Django settings for vendor-comms as an ASGI service with CORS, sentry, pghistory, whitenoise, and the liveness middleware. Uses the shared <code>BaseServiceSettings</code>, <code>setup_sentry</code>, and <code>setup_logging</code>.</p> | |
| <h3>Design Decisions</h3> | |
| <p><code>corsheaders</code> is properly added to both <code>INSTALLED_APPS</code> and <code>MIDDLEWARE</code>. Liveness middleware is placed before SecurityMiddleware (correct, since it needs to bypass <code>ALLOWED_HOSTS</code> checks). pghistory middleware is conditionally excluded in test mode.</p> | |
| </div> | |
| </div> | |
| <div class="diffs"> | |
| <h3>Key Diffs</h3> | |
| <div class="diff-file"> | |
| <div class="diff-file-name">services/vendor-comms/vendor_comms/settings.py (middleware stack)</div> | |
| <pre class="diff-block"><span class="diff-line add">+MIDDLEWARE = [</span> | |
| <span class="diff-line add">+ <span class="str">'corsheaders.middleware.CorsMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'shared_utils.middleware.LivenessMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'django.middleware.security.SecurityMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'django.contrib.sessions.middleware.SessionMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'django.middleware.common.CommonMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'django.middleware.csrf.CsrfViewMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'django.contrib.auth.middleware.AuthenticationMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'django.contrib.messages.middleware.MessageMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'django.middleware.clickjacking.XFrameOptionsMiddleware'</span>,</span> | |
| <span class="diff-line add">+ <span class="str">'whitenoise.middleware.WhiteNoiseMiddleware'</span>,</span> | |
| <span class="diff-line add">+]</span></pre> | |
| </div> | |
| </div> | |
| <div class="concerns"> | |
| <h3>Concerns</h3> | |
| <div class="concern-box issue"><strong>#9 Minor (1/2):</strong> WhiteNoise middleware is placed last in the stack (after XFrameOptionsMiddleware). WhiteNoise docs recommend placing it directly after SecurityMiddleware for best performance.</div> | |
| </div> | |
| </div> | |
| <!-- Block 5 --> | |
| <div class="card" id="block-5"> | |
| <div class="card-header"> | |
| <div class="num">5</div> | |
| <h2>Dependency Management & Build Config</h2> | |
| </div> | |
| <div class="card-body"> | |
| <div class="card-files"> | |
| <ul> | |
| <li><span class="dot dot-modified"></span>pyproject.toml</li> | |
| <li><span class="dot dot-modified"></span>uv.lock</li> | |
| <li><span class="dot dot-modified"></span>tach.toml</li> | |
| <li><span class="dot dot-modified"></span>.gitignore</li> | |
| <li><span class="dot dot-modified"></span>docs/coding-standards.md</li> | |
| <li><span class="dot dot-modified"></span>api/settings.py</li> | |
| </ul> | |
| </div> | |
| <div class="card-desc"> | |
| <h3>What it does</h3> | |
| <p>Removes minimum version pins from several root dependencies and moves them to <code>constraint-dependencies</code>. Adds <code>uvicorn</code> and <code>whitenoise</code> to tach external excludes. Documents dependency management practices in coding-standards.md. Adds <code>staticfiles/</code> to <code>.gitignore</code>.</p> | |
| <h3>Design Decisions</h3> | |
| <p>Moving version floors to <code>constraint-dependencies</code> centralizes version pinning for the monorepo. This is a valid pattern for internal packages that are never published.</p> | |
| </div> | |
| </div> | |
| <div class="concerns"> | |
| <h3>Concerns</h3> | |
| <div class="concern-box issue"><strong>#10 Nit:</strong> Version specifier removal applied inconsistently. Some deps lost their <code>>=</code> specifiers while most others still have them. The coding standards doc (updated in this PR) says "We use <code>>={minimum_version}</code> version specifiers."</div> | |
| </div> | |
| </div> | |
| <!-- Summary Table --> | |
| <div class="summary-table"> | |
| <h2>Action Items</h2> | |
| <table> | |
| <thead> | |
| <tr><th style="width:40px">#</th><th style="width:100px">Severity</th><th>Issue</th></tr> | |
| </thead> | |
| <tbody> | |
| <tr> | |
| <td>1</td> | |
| <td><span class="severity bug">Bug</span></td> | |
| <td><code>services/uvicorn.sh</code>: <code>wait $UVICORN_PID</code> does not re-wait after trap handler; script exits before uvicorn finishes draining, exit code not propagated to k8s</td> | |
| </tr> | |
| <tr> | |
| <td>5</td> | |
| <td><span class="severity design">Design</span></td> | |
| <td><code>django_stubs_ext.monkeypatch()</code> in <code>shared_utils/__init__.py</code> runs at import time, conflicts with <code>api/__init__.py</code> which calls it with <code>extra_classes</code>; side effect in library init is fragile</td> | |
| </tr> | |
| <tr> | |
| <td>6</td> | |
| <td><span class="severity design">Design</span></td> | |
| <td><code>TModel</code> TypeVar defined in both <code>api/types.py</code> and <code>shared_utils/db_utils.py</code>; two separate TypeVars confuse mypy if mixed <em>(1/2)</em></td> | |
| </tr> | |
| <tr> | |
| <td>2</td> | |
| <td><span class="severity minor">Minor</span></td> | |
| <td><code>$DDTRACE</code> unquoted in <code>uvicorn.sh</code>; if unset, command behavior is ambiguous</td> | |
| </tr> | |
| <tr> | |
| <td>3</td> | |
| <td><span class="severity minor">Minor</span></td> | |
| <td>Race window between uvicorn backgrounding (line 78) and trap registration (line 90) in <code>uvicorn.sh</code> <em>(1/2)</em></td> | |
| </tr> | |
| <tr> | |
| <td>4</td> | |
| <td><span class="severity minor">Minor</span></td> | |
| <td><code>sentry-sdk[pymongo]</code> as dep of <code>shared-config</code> forces pymongo extras on all consumers</td> | |
| </tr> | |
| <tr> | |
| <td>9</td> | |
| <td><span class="severity minor">Minor</span></td> | |
| <td>WhiteNoise middleware placed last instead of right after SecurityMiddleware per docs <em>(1/2)</em></td> | |
| </tr> | |
| <tr> | |
| <td>7</td> | |
| <td><span class="severity nit">Nit</span></td> | |
| <td><code>structlog.getLogger</code> in <code>shared_utils/middleware.py</code>; rest of codebase uses <code>structlog.get_logger</code> <em>(1/2)</em></td> | |
| </tr> | |
| <tr> | |
| <td>8</td> | |
| <td><span class="severity nit">Nit</span></td> | |
| <td>Unused <code>_logger</code> in <code>shared_utils/admin.py</code> <em>(1/2)</em></td> | |
| </tr> | |
| <tr> | |
| <td>10</td> | |
| <td><span class="severity nit">Nit</span></td> | |
| <td>Version specifier removal from root deps applied inconsistently with coding standards</td> | |
| </tr> | |
| <tr> | |
| <td>11</td> | |
| <td><span class="severity expected">Expected</span></td> | |
| <td>Admin and middleware classes duplicated between <code>api/</code> and <code>shared_utils/</code>; presumably consolidated in follow-up</td> | |
| </tr> | |
| </tbody> | |
| </table> | |
| </div> | |
| <script> | |
| function scrollToCard(id) { | |
| document.getElementById(id)?.scrollIntoView({ behavior: 'smooth', block: 'start' }); | |
| } | |
| </script> | |
| </body> | |
| </html> |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment