refactor: cut dead surface and duplication found by a repo-wide over-engineering audit - #120
Merged
Merged
Conversation
A repo-wide over-engineering audit turned up surface that no code path can reach and helpers copied per service. Remove the GetMetrics/ServiceMetrics plugin API: 102 of 104 implementations returned a zero-valued struct, and TotalRequests/ErrorCount were never populated by anyone, so /devcloud/api/metrics reported zeros forever. The two services that did fill ResourceCount (DynamoDB, Lambda) are unaffected in practice — /devcloud/api/services already derives resource counts from ListResources. The interface is still pre-1.0, so this is not a v1.x break. Delete the generic shared.ResourceStore (114 lines plus 204 of tests): no production caller ever used it, every service writes its own SQL. Its shared.Scanner was a second declaration of the identical sqlite.Scanner, so configservice now uses the latter. Fold 23 copies of the same string-param accessor (strParam / strVal / getString / str — three spellings, identical behaviour) into shared.StrParam, point 10 per-service region constants at shared.DefaultRegion, and make shared.DefaultAccountID mirror plugin.DefaultAccountID rather than repeat the literal. Left alone deliberately: the per-service jsonError/jsonResponse helpers, whose Content-Type differs by protocol (application/json vs x-amz-json-1.0 vs 1.1), and the randHex/generateID variants, which produce genuinely different id formats. Merge the two mirror-image case-conversion walkers in shared into one mapKeys(v, f) and move it to kafka, its only caller, with a test.
The embedded default config was 325 lines in which all 103 service entries were byte-identical boilerplate: enabled plus data_dir ./data/<name>, with no exceptions. Derive that instead. The services block is now optional and authoritative — omit it and every registered service starts under ./data/<service>; list any service and only the listed ones start, which is the behaviour existing partial configs already relied on. Startup iterates the plugin registry rather than a config map, so a newly registered service no longer needs a YAML entry to run. Delete the event bus and the admin WebSocket at /devcloud/api/ws: nothing in the binary ever called Publish, and main.go dropped the bus reference at the call site, so the socket accepted connections and could never send a message. The REST /devcloud/api/logs endpoint already serves the request log it was meant to stream. This drops the gorilla/websocket dependency. Replace the custom buffering slog.Handler (92 lines) with config warnings returned as a []string and logged after setupLogging — same guarantee that config-time warnings honour logging.format/level, without a bespoke handler. The 'config file not found, using embedded defaults' notice is gone: it fired on the zero-config happy path, and keeping it was the only reason the handler existed. Fold the two init loops, whose bodies were identical, into one closure. The fixed initOrder stays: it encodes core-service-failure-is-fatal, not just the iam-before-sts ordering. Keep the deprecated 'dashboard' key shim — the rename it protects is still unreleased, so dropping it now would silently break v0.2.0 configs. Drop two of the LogCollector's three redundant size clamps, keeping the divide-by-zero guard.
normalizeServiceID went 248 lines to 133. Of its 117 cases, 79 arms were identity mappings (case "s3": return "s3") — but they were not simply redundant: they also lowercased their input, which `default: return svc` did not. The default now lowercases, which makes the identity arms genuinely dead and is strictly better for unmatched names, since every registry key is lowercase. The rewrite was verified by asserting that 385 labels (every case arm, every return value, their uppercase variants, plus unmatched samples) map identically before and after. One dead arm went too: "simpleWorkflowService" could never match a switch on strings.ToLower. serviceFromQueryRequest drops the Action-name whitelist. It only ran for a request carrying neither a SigV4 credential scope nor an iam/sts/sqs host prefix — and every AWS SDK, the CLI, and Terraform sign their requests. With the whitelist gone the QueueUrl probe is also redundant, since it returned the same value as the fallback, so the body argument and the net/url import go with it. Delete gateway/auth.go: ExtractAccountID read the Authorization header into _, returned a constant, and had no callers.
download-smithy-models.sh skipped any model already on disk, and every model is committed, so the weekly job re-ran codegen over unchanged inputs and found nothing every week. Its hand-maintained 40-line SERVICES list had also drifted from upstream naming, so 14 entries (secretsmanager, logs, monitoring, events, route53, apigateway, autoscaling, dms, ...) 404'd on every run. Verified against upstream: the committed sqs and kms models are stale. The list is now derived from the models present, so it cannot drift; name a service explicitly to add a new one. The workflow passes --refresh to re-download, and downloads land through a temp file, so a failed fetch can no longer delete or truncate a committed model — the previous code did rm -f on the destination. The change check moves to git status --porcelain because git diff --quiet cannot see a newly generated (untracked) package. The models stay committed on purpose, and the script now says why: BASE_URL tracks aws-sdk-go-v2 main, so they are the pin that makes `make codegen` reproducible and offline. Only the weekly job refreshes them, which is what turns an upstream API change into a reviewable model diff. codegen now gofmts what it writes, so a fresh `make codegen` is byte-identical to the committed tree instead of showing a whole-tree reformat. That made one thing visible: codegen was resurrecting internal/generated/sts, 563 lines deleted in #91 and #96 because STS is hand-written in internal/services/iam, as untracked files the sync's diff check could not see. STS is now skipped; it is Query-protocol, so it contributes nothing to the JSON-only CRUD registry.
The codegen diagram listed interface.go, serializer.go, and deserializer.go as generated outputs. None of them exist, and README claimed codegen produces "serializers". That is the source of a recurring misreading of internal/generated as scaffolding waiting to be filled in: there is no generated wire glue, providers parse the raw *http.Request themselves, so types.go and base_provider.go have nothing to connect to and only router.go is consumed today. Document the four files actually generated, and why the Smithy models are committed. Also drop the event bus and WebSocket sections, the GetMetrics contract row, the /devcloud/api/metrics endpoint, and the auth.enabled key, all of which describe code that no longer exists; and state that the services config block is optional and authoritative.
CI ran go test ./internal/..., which skipped cmd/devcloud — so the ServicePlugin conformance test over every registered service, the thing that enforces the documented plugin contract, never actually ran. Use ./... . `make stats` counted services by parsing the services block out of default.yaml, which no longer lists them; count the service packages instead.
Contributor
There was a problem hiding this comment.
Sorry @skyoo2003, your pull request is larger than the review limit of 150000 diff characters
Contributor
Reviewer's GuideThis PR removes unused admin metrics/event bus infrastructure and related plugin API surface, consolidates duplicated helpers and constants, simplifies configuration loading/derivation, tightens protocol detection for SigV4/Query services, fixes and hardens the weekly Smithy model sync/codegen workflow, and updates CI, docs, and codegen to match actual usage, all with no intended change to the AWS API behaviour. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
A code review of the refactor commits on this branch turned up one behaviour regression and four silent failures. serviceFromQueryRequest dropped its Action fallback on the reasoning that every SDK, the CLI, and Terraform sign their requests. They do — but the fallback only ever ran for a request with neither a SigV4 credential scope nor an iam/sts/sqs host prefix, and for those the function now returned "sqs" unconditionally: an unsigned Action=GetCallerIdentity POST to a bare endpoint reached the SQS provider. The existing Query tests set Host to iam./sts., so they never covered the path they were meant to. The fallback is back, ordered after SigV4 and the host prefix and matching on IAM entity substrings rather than the old 35-name whitelist, which makes it shorter and wider — DeleteRole and ListPolicies were both missing from that list. sqs stays the final default. download-smithy-models.sh counted failures and exited 0, so the weekly sync would regenerate from stale models, see no diff, and report a successful sync of nothing — the same silent no-op the previous commit set out to fix. It now exits 1 when any download failed. Its model count also moves from `ls *.json` to find: under set -euo pipefail a glob matching nothing made ls exit 2 and killed the script just before it printed the summary. The removed auth block is parsed again, only to warn. yaml.Unmarshal ignores unknown keys, so an operator who wrote auth.enabled: true to require signature validation got no warning that nothing checks signatures — the one deprecation that must not be silent. Follows the dashboard key's one-release shim. A services block is authoritative and Enabled is a plain bool, so a block that lists a service without enabled: true starts zero services with nothing in the log. main now warns when the active set is empty, which also covers a typo'd DEVCLOUD_SERVICES. docs/configuration.md claimed enabled defaults to true; it defaults to false and is required per entry. Also: make stats counted service directories and reported 103, because STS lives in internal/services/iam — count registry registrations instead, which gives the 104 README states. Finish the shared.StrParam de-duplication with StrParamDefault, removing five more copies (29 call sites). Drop the now-unreachable "simpleWorkflowService" case label, since the switch lowercases its input. Stop copying the request body into a string to test for "Action=". Merge the split import groups left in twelve providers. Isolate DEVCLOUD_* from the process environment in the config tests, which otherwise fail on a machine that exports them. Left alone deliberately: the eleven services with no committed Smithy model (account, cloudcontrol, dms, ...). The script takes an arbitrary MODELS_DIR and must not know about internal/services; the invariant that every service package has a model belongs in a codegen test. intParam stays duplicated — two of its six copies differ in signature and integer width, so folding them would change behaviour at the call site.
The fragments had grown into commit messages: line counts, per-service tallies, and the reasoning behind each change. A CHANGELOG reader wants what changed and why it mattered; the rest is already in git. Trimmed to one or two sentences each, keeping every name a reader would grep for — config keys, endpoints, package paths, operation names.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A repo-wide over-engineering audit, applied: removes surface no code path can reach, folds per-service copies of the same helper into one, and fixes a weekly workflow that was structurally incapable of doing its job. Net -1,951 lines and one fewer dependency, with no intended behaviour change to the AWS API surface.
Related Issue
None — no tracking issue; the findings are listed below.
Changes
Dead surface removed
GetMetrics/ServiceMetricsplugin API and the/devcloud/api/metricsendpoints — 102 of 104 implementations returned a zero-valued struct and nobody ever populatedTotalRequests/ErrorCount, so the endpoint reported zeros forever. The two services that did fillResourceCountlose nothing:/devcloud/api/servicesalready derives counts fromListResources. Pre-1.0, so not av1.xbreak./devcloud/api/ws— nothing in the binary ever calledPublish, andmain.godropped the bus reference at the call site, so the socket accepted connections and could never send a message. Dropsgorilla/websocket.shared.ResourceStore(114 lines + 204 of tests) — no production caller; every service writes its own SQL. Itsshared.Scannerwas a second declaration of the identicalsqlite.Scanner.gateway.ExtractAccountID— read theAuthorizationheader into_, returned a constant, no callers.auth.enabledkey — its only effect was a startup warning that it was unimplemented.slog.Handler(92 lines) — replaced by config warnings returned as[]stringand logged aftersetupLogging, keeping the same guarantee from fix: config warnings honor logging.format/level (#113) #119 that config-time warnings honourlogging.format/level.Duplication folded
strParam/strVal/getString/str— three spellings, identical behaviour) →shared.StrParam; 10 per-service region constants →shared.DefaultRegion;shared.DefaultAccountIDnow mirrorsplugin.DefaultAccountID.shared→ onemapKeys(v, f), moved to kafka, its only caller, with a test.jsonError/jsonResponsehelpers, whose Content-Type differs by protocol (application/jsonvsx-amz-json-1.0vs1.1), and therandHex/generateIDvariants, which produce genuinely different id formats.Config
default.yaml325 → 16 lines. All 103 service entries were byte-identical boilerplate (enabled+data_dir ./data/<name>) with zero exceptions, so they are derived now. Theservicesblock is optional and authoritative: omit it and every registered service starts; list any service and only the listed ones start — the behaviour existing partial configs already relied on.dashboardkey shim stays: the rename it protects is still unreleased, so dropping it now would silently break v0.2.0 configs.Protocol detection
normalizeServiceID248 → 133 lines. 79 of 117 case arms were identity mappings — but not simply redundant: they also lowercased their input, whichdefault: return svcdid not. The default now lowercases. Verified by asserting 385 labels (every case arm, every return value, uppercase variants, unmatched samples) map identically before and after. One arm,"simpleWorkflowService", could never match a switch onstrings.ToLower.serviceFromQueryRequestdrops the Action-name whitelist: it only ran for a request with neither a SigV4 credential scope nor aniam/sts/sqshost prefix, and every SDK, the CLI, and Terraform sign their requests.Weekly Smithy sync — was a no-op
download-smithy-models.shskipped any model already on disk, and all 93 are committed, so the job re-ran codegen over unchanged inputs and found nothing every week. Its hand-maintained 40-line service list had drifted from upstream naming, so 14 entries (secretsmanager,logs,monitoring,events,route53,apigateway, ...) 404'd on every run. Verified against upstream: the committedsqsandkmsmodels are stale.--refresh; downloads go through a temp file so a failed fetch can no longer delete a committed model (the old code didrm -fon the destination); the change check moves togit status --porcelain, whichgit diff --quietcannot do for a newly generated untracked package.BASE_URLtracksaws-sdk-go-v2main, so they are the pin that makesmake codegenreproducible and offline.make codegenis byte-identical to the committed tree. That exposed codegen resurrectinginternal/generated/sts— 563 lines deleted in refactor: remove dead code and de-duplicate service boilerplate #91 and feat: Phase 1 AWS depth & stabilization for v1.0 #96 — as untracked files the diff check could not see. STS is now skipped.CI / docs
go test ./internal/..., skippingcmd/devcloud, so theServicePluginconformance test that enforces the documented plugin contract never ran. Now./....interface.go,serializer.go,deserializer.goas generated outputs; none exist, and README claimed codegen produces "serializers". Corrected — this was the source of a recurring misreading ofinternal/generatedas scaffolding waiting to be filled in.Not in this PR
internal/generated/is 240,793 lines across 93 packages, of which 88 (224,650 lines) have no importer at all. I looked at whether that is pending implementation work rather than dead weight, and it is not:parser.goparseshttpLabel/httpHeader/httpQuery/httpPayload, but no generator or template reads those fields — onlymodel.goandparser_test.gomention them. Providers get the raw*http.Requestand parsemap[string]any, sotypes.goandbase_provider.gohave nothing to connect to.errors.gois not adoptable as-is: 1,015 of 2,728 generated error types reportHTTPStatus() 0.router.gois only meaningful for REST services: 34 of 92 packages have any non-empty URI pattern; the rest emit{Method: "", Pattern: ""}rowsMatchOperationcan never match.docs/crud-engine.mdsays the promotion path is "implement it as an explicitcasein the service provider" — not the typedBaseProvider. Phase 2 is the IR/ModelSourcerefactor; no serializer work is planned.Suggested follow-up, for a maintainer decision rather than this PR: generate
router.goonly, and only for the 34 services with real URI patterns; stop generatingtypes.go/errors.go/base_provider.go; un-embedBaseProviderfrom bedrock (which never calls it) and efs. Roughly 209k lines, with codegen still driving everything actually consumed.Test Plan
go build ./...,go vet ./...,go test ./...— green (./..., which now includes the conformance test).golangci-lint run— 0 issues.DEVCLOUD_BIN=dist/devcloud pytest tests/compatibility/). This exercises the riskiest changes end to end — the derived service config (the server started from embedded defaults with noservicesblock), protocol detection, and theStrParamrewrite across 23 services.make codegenover all 93 models →git statusclean, i.e. byte-identical to the committed tree.normalizeServiceIDrewrite checked by differential assertion over 385 labels.admin.enabled: trueandlogging.format: json: 104 services initialise, thedashboarddeprecation warning is emitted as JSON after logging is configured (confirming the fix: config warnings honor logging.format/level (#113) #119 guarantee survives without the buffer handler),/servicesand/logsrespond,/metricsand/wsreturn 404, and data lands in./data/<service>.download-smithy-models.sh --refreshtested against upstream in a temp directory: updates real models, and a 404 leaves the existing file untouched.Checklist
golangci-lint run)changie new, see docs/release.md) — or N/A (docs/tests/chore only)