Add Caching Functionality when Fetching Issue Labels for Deployment Reviews #5
Conversation
- Add in-memory cache for issue labels - Add cache initialization methods - Add cache hit/miss logging - Add cache state management - Support optional cache bypass
|
All tests passing: Cache initialized with 77 open issues: 77 Open issues in Octodemo at the same time I ran locally: 4 Issues in Warn State >7 days old, which matches what I see in the UI: |
|
@davelosert Apologies for all this dist files, should I have separated these out in a different PR? |
There was a problem hiding this comment.
Hey @alecbuchanan,
thank you again for this PR. I left a few minor comments, but all in all, gerat job!
The dist-folders are fine - they even need to be generated within the PR in order for a clean release. So please, after your changes, rebuild and recommit the dist file as well! :)
| let manager: GitHubDeploymentManager; | ||
|
|
||
| beforeEach(() => { | ||
| vi.clearAllMocks(); |
There was a problem hiding this comment.
Can we put this into a "afterEach" call for cleaness?
| open: 'open', | ||
| closed: 'closed', | ||
| all: 'all', | ||
| } as const; |
There was a problem hiding this comment.
I would define this only as type and derive it directly from the GitHub API.
We only use it as non-type in the GitHubDeploymentManager, and for one spot I believe an ENUM-kind-of-definition is too much.
Given they GitHubDeploymentManager already has a dependency to octokit.js, lets derive the issue state from there and then just use a string as the default value (I will leave a comment on those code references as well.)
| } | ||
|
|
||
| async fetchLabelsForRepo( | ||
| state: keyof typeof ISSUE_STATES = ISSUE_STATES.open, |
There was a problem hiding this comment.
As described in my comment for constants.ts, let's not use a pseudo-enum here but let's keep things simple:
Derive the issue-state type from the octokit types and then just use a string for the default definition here. it would look something like this:
type OctokitType = InstanceType<typeof Octokit>;
type Issue = Awaited<
ReturnType<OctokitType["rest"]["issues"]["listForRepo"]>
>["data"][0];
type IssueState = Issue["state"];
[...]
async fetchLabelsForRepo(
state: IssueState = 'open'
[...]| } | ||
|
|
||
| private async initializeLabelCache( | ||
| state: keyof typeof ISSUE_STATES = ISSUE_STATES.open, |
| description?: string | null; | ||
| color?: string | null; | ||
| default?: boolean; | ||
| } |
There was a problem hiding this comment.
Derive it from Octokit here as you did in the LabelMockFactory.ts file
Addresses https://github.com/github/fs-tech-tooling/issues/85
Enhancements to
GitHubDeploymentManager:fetchLabelsForIssueandfetchLabelsForRepoto fetch labels for a specific issue and all issues in a repository, respectively. [1] [2]getIssueLabelsandinitializeLabelCacheto improve performance by caching labels. [1] [2]Updates to constants and types:
ISSUE_STATESconstant to represent different issue states (open, closed, all).GitHubLabelinterface to define the structure of a GitHub label.Testing improvements:
GitHubDeploymentManagerto cover label fetching and caching functionalities.LabelMockFactoryto generate mock labels and responses for testing purposes.