fix: getEnvironmentsToBuild queried a relation removed in phase 1 - #231
Open
vigneshrajsb wants to merge 1 commit into
Open
fix: getEnvironmentsToBuild queried a relation removed in phase 1#231vigneshrajsb wants to merge 1 commit into
vigneshrajsb wants to merge 1 commit into
Conversation
The dead-code cleanup (#228) removed Environment.relationMappings.services along with the rest of the legacy DB-config path, but left this call site: Environment.find().withGraphJoined('services').where('services.repositoryId', ...) Objection throws UnknownRelationError: services when it runs. It is reached from createBuildAndDeploys -> getEnvironmentsToBuild whenever environmentId is null, i.e. a repository whose defaultEnvId is not set (github.ts passes repository?.defaultEnvId straight through), so it has been latent rather than loud. That else-branch was the DB-service-based environment lookup: "find the environments that own a service belonging to this repo". With YAML config a repository's environment is its defaultEnvId, so the branch has no meaning anymore and is removed rather than repaired. Returning an empty array is what the caller already expects - it logs "no matching environments" and returns. Also stops pushing a possibly-undefined findOne() result into the array, which would surface later as an undefined element during environments.map(). Adds regression coverage that fails on the previous implementation: the Environment.find mock throws if called, and the missing-environment case uses toStrictEqual so [undefined] cannot pass as [].
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
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
Phase 1 of the dead-code cleanup (#228) removed
Environment.relationMappings.servicesalong with the rest of the legacy DB-config path, but left one call site behind ingetEnvironmentsToBuild:Objection throws
UnknownRelationError: serviceswhen this runs.Impact
Latent, not loud. The branch only executes when
environmentIdis null — i.e. a repository whosedefaultEnvIdis not set (github.tspassesrepository?.defaultEnvIdstraight through tocreateBuildAndDeploys). Repositories onboarded normally have adefaultEnvId, which is why production has been quiet since #228 merged.Fix
That
elsebranch was the DB-service-based environment lookup — "find the environments that own a service belonging to this repo". Under YAML config a repository's environment is itsdefaultEnvId, so the branch is meaningless now and is removed rather than repaired. Returning[]is exactly what the caller already handles:Also stops pushing a possibly-
undefinedfindOne()result into the array, which would otherwise surface later as an undefined element duringenvironments.map(...).Testing
BuildService getEnvironmentsToBuildsuite, verified to fail on the previous implementation and pass on this one. TheEnvironment.findmock throws if called, and the missing-environment case usestoStrictEqualso[undefined]cannot pass as[].lintclean.ts-check: 291 errors onmain, 291 on this branch — zero new.Found while re-verifying scope for phase 2 of the cleanup.