ref: Decouple functionality from the registry#168
Conversation
Add more docs to describe more meaningfully the functionality some of the `registry`'s methods do.
|
I didn't look at the code yet but just FYI it would be nice if we can maybe also fix that. |
|
@HazAT Please check the comment here #167 (comment) |
tonyo
left a comment
tonyo
left a comment
There was a problem hiding this comment.
Some of the things that were extracted seem too tied to the registry target still, which makes them less reusable or general-purpose (for example, functions addPackageVersionToRegistry and createSymlinks). I don't have good suggestions how to decouple them better, but let's at least move all these new modules to the registry submodule (so it'll be utils/registry), to illustrate that they're not really useful outside interacting with the release registry.
For functions that are more generic (e.g., git operations), please check if we already use similar snippets in other files, and whether it makes sense to start using your new util functions or not.
|
@iker-barriocanal it would also be great to tie this to some existing issues or planned future work to ensure this is not refactoring for refactoring sake. This seems like a pretty large change and I'd love to have a very good reason behind it before taking the risk. |
|
@BYK at the moment, updates on the registry are done through the registry target. However, when new AWS Lambda layers are created, their information also needs to be added (see getsentry/sentry-release-registry#30). It's easier to avoid code duplication if some functionality is decoupled. |
|
@tonyo some of the things can (and will) be more easily be used by other modules (e.g. Other modules (e.g. |
BYK
left a comment
BYK
left a comment
There was a problem hiding this comment.
Added some nits but LGTM and can be merged as is.
| * @param canonicalName Registry canonical name | ||
| * @returns A list of directories | ||
| */ | ||
| export function parseCanonical(canonicalName: string): string[] { |
There was a problem hiding this comment.
May want to replace this with https://github.com/npm/npm-package-arg
| if (!checksums) { | ||
| return []; | ||
| } | ||
| if (!Array.isArray(checksums)) { |
There was a problem hiding this comment.
Shouldn't this be checked via TypeScript?
| !Object.values(HashAlgorithm).includes(item.algorithm) || | ||
| !Object.values(HashOutputFormat).includes(item.format) |
There was a problem hiding this comment.
I would move these out of the loop:
const VALID_ALGORITHMS = new Set(Object.values(HashAlgorithm));
const VALID_OUTPUT_FORMATS = new Set(Object.values(HashOutputFormat));
## Summary Resolves **10 open Dependabot alerts** across 5 CVEs and dismisses 2 low-severity esbuild alerts as tolerable risk. ### Alerts Fixed | Alert(s) | Package | Severity | CVE | Fix | |----------|---------|----------|-----|-----| | #180, #181 | `tar` | Medium | GHSA-vmf3-w455-68vh | Bump direct pin `7.5.11` -> `7.5.16` | | #178, #179 | `form-data` | High | GHSA-hmw2-7cc7-3qxx | `pnpm.overrides` for `4.0.6` (v4) and `2.5.6` (v2) | | #172, #176 | `vite` (root) | High + Medium | GHSA-fx2h-pf6j-xcff, GHSA-v6wh-96g9-6wx3 | Add as direct devDep `^7.3.5` + override | | #173, #177 | `vite` (docs) | High + Medium | same | `pnpm.overrides` in docs | | #174, #175 | `astro` | High + Medium | GHSA-2pvr-wf23-7pc7, GHSA-jrpj-wcv7-9fh9 | `pnpm update` -> `6.4.8` | ### Alerts Dismissed (tolerable_risk) | Alert(s) | Package | Severity | Reason | |----------|---------|----------|--------| | #167, #168 | `esbuild` | Low | GHSA-g7r4-m6w7-qqqr - Windows dev-server only vulnerability. Craft is a CLI tool, docs is a static site. Transitive dep of `[email protected]` which pins `esbuild@^0.27.0` - cannot fix without vite 8 major bump. | ## Changes ### Root project (`package.json`) - `tar`: `7.5.11` -> `7.5.16` (direct pin bump) - `vite`: added as devDependency at `^7.3.5` (was only transitive via vitest) - `pnpm.overrides`: added `form-data@>=4: ^4.0.6`, `form-data@<3: ^2.5.6`, `vite: ^7.3.5` ### Docs project (`docs/package.json`) - `astro`: resolved `5.16.11` -> `6.4.8` (specifier `^6.1.10` allowed it) - `@astrojs/starlight`: resolved `0.37.3` -> `0.38.3` - `pnpm.overrides`: added `vite: ^7.3.5` ## Verification - `pnpm build` - passed - `pnpm test` - 1025 passed, 1 skipped - `pnpm lint` - 0 errors - `pnpm docs:build` - 27 pages built successfully
4 participants
This PR is not adding any functionality, but refactoring the
registrytarget to decouple some functionality, so that there are more pieces with smaller responsibility each.Currently the target
aws-lambda-layer, after creating new layers, only logs to console the layer's ARNs; but we want to have the data of the created new layers in the registry. However, only theregistrytarget is able to write there. In order to have the targets independent between them, the goal is to make theaws-lambda-layertarget able to write to the registry, so that it doesn’t need to interact with theregistrytarget. This PR is focused on the refactoring part of that small feature. Besidesaws-lambda-layer, additional serverless targets might be created in the future.Aiming to leave the codebase a bit better and make the code more readable and usable, additional refactoring has been made not to have the
registrytarget do that many things. Thus, some functionality theregistrytarget does has been broken down into other modules (again, some of the modules won't be used in the new feature of theaws-lambda-layertarget, and will only be used by theregistrytarget for now).This refactoring of decoupling functionality from the registry target (
src/targets/registry) includes:src/utils/checksumsmodule.src/utils/symlinkmodule.src/utils/packagePathmodule.src/utils/packagePathmodule.utilsfor theregistrytarget, in thesrc/utils/registrydirectory.