[WIP] feat: add new builders for metadata migration check #1732
Conversation
cpourcel
force-pushed
the
feature/metadata-migration-check
branch
from
815ce00
to
3a39949
Compare
cpourcel
changed the title
cpourcel
force-pushed
the
feature/metadata-migration-check
branch
from
3a39949
to
7b5fd7d
Compare
| "type": "object", | ||
| "$id": "ConfigMigrationMetadataCheckBuilderSchema", | ||
| "title": "Check config migration metadata builder", | ||
| "description": "", |
| "type": "string", | ||
| "description": "Path to the file containing the migration metadata." | ||
| }, | ||
| "granularity": { |
There was a problem hiding this comment.
may have an enum in json schema contrain
| "allowBreakingChanges": { | ||
| "type": "boolean", | ||
| "description": "Are breaking changes allowed.", | ||
| "default": true |
There was a problem hiding this comment.
I would default to false personally
| "type": "string", | ||
| "description": "Migration metadata name override, if not provided will use the one with the highest version following the pattern." | ||
| }, | ||
| "toVersion": { |
There was a problem hiding this comment.
to be aligned with ngUpdate options, I would use from and to instead
| "title": "Check config migration metadata builder", | ||
| "description": "", | ||
| "properties": { | ||
| "migrationMetadataFolder": { |
There was a problem hiding this comment.
I would take a pattern(s) (list?) instead of a folder:
This would allow to handle a full folder but also filter inside if needed and support multi path
| * @param metadataItem Metadata item | ||
| * @param migrationItem Migration item | ||
| */ | ||
| isMigrationDataMatch(metadataItem: T, migrationItem: any): boolean; |
There was a problem hiding this comment.
What should I use if I don't know the type of metadata that I will get here ? I could add a generic type like MetadataComparator<T, K> but it seems a bit too much
| migrationMetadataFolder: string; | ||
|
|
||
| /** Granularity of the migration check. */ | ||
| granularity: 'major' | 'minor'; |
There was a problem hiding this comment.
Patch is not available on purpose?
There was a problem hiding this comment.
It's not in the requirements but I think I can add it. I will think about it to check that there would be no issue with that
| */ | ||
| function getPackageManagerForRegistry(options?: PackageManagerOptions): SupportedPackageManagers | undefined { | ||
| const packageManagerInfo = getPackageManagerInfo(options); | ||
| if (!packageManagerInfo.version) { |
There was a problem hiding this comment.
If it is npm without version, do we want to return undefined?
| * @param context Builder context (from another builder) | ||
| * @param comparator Comparator implementation, depends on the type of metadata to check | ||
| */ | ||
| export async function checkMetadataBuilder<T>(options: MigrationMetadataCheckBuilderOptions, context: BuilderContext, comparator: MetadataComparator<T>): Promise<BuilderOutput> { |
| }; | ||
| } | ||
|
|
||
| // TODO: should be an input? |
| if (errors.length) { | ||
| return { | ||
| success: false, | ||
| error: errors.reduce(((message, error) => message.concat(error.message, '\n')), '') |
There was a problem hiding this comment.
| error: errors.reduce(((message, error) => message.concat(error.message, '\n')), '') | |
| error: errors.map({message}) => message).join(os.EOL) |
| return npmFileExtractor.getFilesNpm(packageRef, filePaths); | ||
| } | ||
|
|
||
| // TODO : there is probably already an utility for this? |
There was a problem hiding this comment.
To read a JSON no, but take care, you should not use fs in a schematic context, you should access to the file via the tree.readJson
| * Read and parses a JSON file | ||
| * @param metadataPath Path of the file | ||
| */ | ||
| export function getLocalMetadataFile<T>(metadataPath: string): T { |
There was a problem hiding this comment.
Should add constraint on T (like JsonObject from type-fest for example)
| * @param filename Filename containing a version | ||
| * @param pattern Regex with a capturing group containing the version | ||
| */ | ||
| export function getVersionFromFilename(filename: string, pattern: RegExp): string | undefined { |
There was a problem hiding this comment.
not sure you need a function to execute a regexp
| plugins: new Set(['plugin-npm']), | ||
| modules: new Map([['plugin-npm', npmPlugin.default]]) | ||
| }; | ||
| const configuration = await Configuration.find(npath.toPortablePath(process.cwd()), pluginConfiguration, { strict: false }); |
There was a problem hiding this comment.
process.cwd() should be a parameter of the function
| } | ||
|
|
||
| /** | ||
| * Retrieves the list of given files from an npm package using yarn. |
There was a problem hiding this comment.
Maybe you should specify that this is for a package not part of the dependencies that you download as well.
Because according to the description it seems to a simple fs.readFile(require.resolve(path))
| * Interface describing a localization migration element | ||
| */ | ||
| export interface MigrationLocalizationMetadata { | ||
| key: string; |
| @@ -0,0 +1,48 @@ | |||
| import { CssMetadata, CssVariable } from '@o3r/styling'; | |||
There was a problem hiding this comment.
| import { CssMetadata, CssVariable } from '@o3r/styling'; | |
| import type { CssMetadata, CssVariable } from '@o3r/styling'; |
|
The following items are missing to the feature:
|
cpourcel
force-pushed
the
feature/metadata-migration-check
branch
from
7b5fd7d
to
105a0e8
Compare
| }, | ||
| "granularity": { | ||
| "type": "string", | ||
| "description": "Granularity of the migration check.", |
There was a problem hiding this comment.
add in parenthesis (minor, major) in the description
There was a problem hiding this comment.
Actually the information will appear if on --help if enum is specified, adding parenthesis may not be required
(cf. comment r1584022846)
| "properties": { | ||
| "migrationMetadataFolder": { | ||
| "type": "string", | ||
| "description": "Path to the file containing the migration metadata." |
| "allowBreakingChanges": { | ||
| "type": "boolean", | ||
| "description": "Are breaking changes allowed.", | ||
| "default": true |
| "description": "Path of the localization metadata file.", | ||
| "default": "./localization.metadata.json" | ||
| }, | ||
| "migrationMetadataName": { |
| }, | ||
| "fromVersion": { | ||
| "type": "string", | ||
| "description": "Override the previous version number to use. If not provided, it will be the latest package available from npm < toVersion and matching the granularity." |
There was a problem hiding this comment.
format of the version ?
| "type": "string", | ||
| "description": "Migration metadata name override, if not provided will use the one with the highest version following the pattern." | ||
| }, | ||
| "toVersion": { |
| "type": "string", | ||
| "description": "Override the version to consider as the latest one for metadata comparison. If not provided, the latest migration metadata file will be considered as the latest version number." | ||
| }, | ||
| "fromVersion": { |
| if (isInNewMetadata) { | ||
| if (!isBreakingChangeAllowed) { | ||
| errors.push(new Error(`Property ${comparator.getName(lastValue)} is not present in the new metadata and breaking changes are not allowed`)); | ||
| break; |
There was a problem hiding this comment.
continue instead of break
Proposed change
Add 3 new builders in @o3r/component, @o3r/styling and @o3r/localization, check-config-migration-metadata, check-localization-styling -metadata and check-localization-migration-metadata
The goal of these builders is to retrieve a previous version of the app (provided as an option or computed from the folder of migration metadata) and compare the current metadata with the previous one. It will raise some errors if breaking changes are detected and they are not allowed, or if these changes are not documented in the provided migration metadata.
Related issues