Fix a circular dependency that led a minimum version to be undefined - #1578
henrymercer merged 2 commits into
Conversation
Sorry, something went wrong.
angelapwen
left a comment
There was a problem hiding this comment.
Thanks for investigation & extra debug statements 🕵️
Sorry, something went wrong.
| repositoryNwo: RepositoryNwo, | ||
| tempDir: string, | ||
| logger: Logger | ||
| private readonly logger: Logger |
There was a problem hiding this comment.
Why do we make this private readonly?
Sorry, something went wrong.
There was a problem hiding this comment.
This makes it a field on the Features class so we can access it within getValue.
Sorry, something went wrong.
There was a problem hiding this comment.
We could also add a logger argument to getValue, but that would cause a reasonable amount of churn, and sharing the logger makes sense.
Sorry, something went wrong.
There was a problem hiding this comment.
Ah, got it, thank you
Sorry, something went wrong.
A circular dependency between codeql.ts and feature-flags.ts meant that featureConfig[Feature.ExportCodeScanningConfigEnabled].minimumVersion was undefined. This in turn meant that we tried to pass some CLI options even if we were running with an early version of the CLI where these options didn't exist.
The fact that we didn't catch this in PR checks highlights the need for more comprehensive testing across the full range of minor versions we support.
Merge / deployment checklist