Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor the store to share code #959

Closed
cmwylie19 opened this issue Jul 14, 2024 · 3 comments · Fixed by #1259
Closed

refactor the store to share code #959

cmwylie19 opened this issue Jul 14, 2024 · 3 comments · Fixed by #1259
Assignees
Labels

Comments

@cmwylie19
Copy link
Collaborator

Describe what should be investigated or refactored

The controller Store has several nested function that are extremely similar, I think we can refactor the store code and reduce a lot of duplicated code.

Links to any relevant code

https://github.com/defenseunicorns/pepr/blob/main/src/lib/controller/store.ts

Additional context

Add any other context or screenshots about the technical debt here.

@samayer12
Copy link
Collaborator

samayer12 commented Oct 10, 2024

Test coverage of store.ts is pretty low, I'll want to shore that up before I can confidently refactor. Here's a snippet from npm run test:unit:

File                  | % Stmts | % Branch | % Funcs | % Lines | Uncovered Line #s
  store.ts            |   16.21 |    12.82 |   17.39 |   16.55 | 19-300

@samayer12
Copy link
Collaborator

After working on this for a bit, it seems like natural spots to refactor are:

  • Code Coverage in store.ts
  • Shared-ish caching logic for two different caches handled by the store
  • Consolidating redaction logic to the logger, which is used by the store

@samayer12
Copy link
Collaborator

#1259 addresses this issue. In general, we introduced the concept of a storeCache for cache operations.

Two outstanding items for future work:

  • Ensure meaningful test coverage of store.ts, which only exposes a constructor
  • Extract migration logic from store.ts to storeCache.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants