Repository navigation
Manage integrity checkers on repo types from the CLI - #1105
Conversation
| static final String INTEGRITY_CHECK_DESCRIPTION = | ||
| "Replaces integrity checkers for this repo type, comma separated format: \"FILE_EXTENSION_1:CHECKER_TYPE_1,FILE_EXTENSION_2:CHECKER_TYPE_2\"\n " | ||
| + "Omitting the flag leaves existing checkers unchanged. An empty value clears them.\n " | ||
| + "Available Checker types: " | ||
| + IntegrityCheckerCli.AVAILABLE_CHECKER_TYPES_TOKEN | ||
| + "\n " | ||
| + "For example: \"properties:MESSAGE_FORMAT,properties:TRAILING_WHITESPACE\""; |
There was a problem hiding this comment.
[Suggestion] Add a help-text test for the {availableCheckerTypes} placeholder
Files: cli/.../L10nJCommander.java (L199–210), RepoCommand.java (L16–22), RepoTypeCreateCommand.java (L73–78), RepoTypeUpdateCommand.java (L77–83)
This also changes the help output of repo-create and repo-update, but nothing tests the substitution. If a future usage path skips L10nJCommander.usage(String), users will see a literal {availableCheckerTypes}. Could we add a test that runs repo-create -h and repo-type-create -h and asserts that:
- the output does not contain
{availableCheckerTypes} - the output contains a type that was missing from the old hardcoded list (e.g.
FLUENTorMISC_AI_TRANSLATE)
Also worth mentioning in the PR description: the help list now shows all 21 enum values instead of the previous 13.
There was a problem hiding this comment.
Added help tests for repo-create -h and repo-type-create -h. Both go through L10nJCommander.usage, which replaces {availableCheckerTypes} with every IntegrityCheckerType.
The tests assert the placeholder is gone and that the output includes types missing from the old hardcoded list (FLUENT, MISC_AI_TRANSLATE). I also noted in the PR description that command help now lists every checker type from the enum, including those that were missing before.
| String[] param = StringUtils.delimitedListToStringArray(integrityCheckerParam, ":"); | ||
| if (param.length != 2) { | ||
| throw new ParameterException( | ||
| "Invalid integrity checker format [" + integrityCheckerParam + "]"); | ||
| } | ||
| String fileExtension = param[0]; | ||
| String checkerType = param[1]; | ||
| IntegrityChecker integrityChecker = new IntegrityChecker(); | ||
| integrityChecker.setAssetExtension(fileExtension); | ||
| try { | ||
| integrityChecker.setIntegrityCheckerType(IntegrityCheckerType.valueOf(checkerType)); | ||
| } catch (IllegalArgumentException ex) { | ||
| throw new ParameterException("Invalid integrity checker type [" + checkerType + "]"); | ||
| } |
There was a problem hiding this comment.
[Suggestion] Throw CommandException from IntegrityCheckerCli.parse instead of ParameterException
Files: cli/.../IntegrityCheckerCli.java (L58–71), RepoTypeCreateCommand.java (L101–102), RepoTypeUpdateCommand.java (L126–127)
parse declares throws CommandException, but it actually throws the unchecked ParameterException. The repo-type commands catch and wrap it, so they print a short error. The repo-* commands don't, so repo-create -it properties:NOPE still ends up in L10nJCommander's catch (Throwable) and prints "Unexpected error" plus a stack trace.
Since the parser is shared now, throwing CommandException directly would give both command families the same short error and remove the two catch (ParameterException ex) blocks. Fine as a follow-up if you'd rather keep repo-* behaviour unchanged in this PR.
There was a problem hiding this comment.
I'd rather keep repo-* behavior unchanged in this PR
I checked all callers and the repo-* commands already handle this case. Both RepoCreateCommand and RepoUpdateCommand catch ParameterException and wrap it in CommandException, so an invalid checker produces the same short CLI error and does not reach L10nJCommander’s catch (Throwable).
That handling predates this PR. The new repo-type-* callers perform the same mapping explicitly. ParameterException also remains consistent with the existing CLI parsers for malformed parameter values, so I’d prefer to keep the current exception type.
I added explicit regression tests for invalid checkers on repo-create and repo-update, but changing the exception type would not change the user-visible behavior.
| - At least one of `--new-name`, `--description`, `--ai-prompt`, `--ai-prompt-file`, or `--integrity-check` / `-it` is required; otherwise `Must provide at least one of the following options: --new-name, --description, --ai-prompt, --ai-prompt-file, --integrity-check`. | ||
| - PATCH body sets only the fields the user passed; omitted flags stay `null` so the server leaves those columns unchanged (see [PATCH semantics](#6-patch-null-means-leave-unchanged)). | ||
| - Checkers: `integrityCheckers` is left Java `null` so `NON_NULL` omits the property (unchanged). A non-null empty set would serialize as `[]` and clear them. There is no checkers CLI flag yet. | ||
| - Omitted `--integrity-check` / `-it` leaves Java `integrityCheckers` null, so `NON_NULL` omits the property and existing checkers stay. A present value replaces the full set, same `FILE_EXTENSION:CHECKER_TYPE` list as create. The same pair twice, including a leading dot on the extension, is stored once. An empty value sends `[]` and clears every checker. An unknown checker type fails before the request with `Invalid integrity checker type [<type>]`. |
There was a problem hiding this comment.
[Suggestion] Validate --integrity-check before looking up the repo type in repo-type-update
Files: cli/.../RepoTypeUpdateCommand.java (L106–111), docs/internal/Architecture.md (Update section, ~L401)
commandHelper.findRepoTypeByName(nameParam) runs before IntegrityCheckerCli.parse(...). So a bad -it value still costs a GET, and if the type name is also wrong, the user sees "not found" instead of the input error. Parsing (and resolving the AI prompt) before the lookup would fail fast with no network call, and would make the doc line "An unknown checker type fails before the request" accurate for update as well as create.
There was a problem hiding this comment.
@kkuliczkowski-box
Thanks — the lookup-before-local-validation order predates this change.
repo-type-update already resolves the existing type before validating the AI prompt or reading the prompt file.
repo-update follows a similar lookup-before-parse sequence.
This PR preserves existing behavior while adding integrity-check parsing.
Moving only checker validation would make validation order inconsistent, while moving all local validation would be a broader behavioral refactor.
I’ll clarify the architecture wording to say the invalid checker fails before the PATCH request.
There was a problem hiding this comment.
Done. The Update section now says an unknown checker type fails before the PATCH request with Invalid integrity checker type [<type>]. The name lookup can still issue a GET first.
…epo create. Build the available checker names from the enum so the help text stays aligned with the types the CLI accepts.
Omitting the flag leaves the set unchanged, and an empty value sends an empty list so the type's checkers are cleared.
Print each extension and checker type on one line, and omit that line when the type has none.
…erbose. The default line is a count so a long catalog stays short, and --verbose prints each extension and checker type.
…er the name lookup.
…s a short CLI error.
…eft as a placeholder. repo-create -h and repo-type-create -h must not print the placeholder and must include checker types missing from the old hardcoded list.
… record that the CLI sets them. Also lock the singular list count and drop the unrelated .conduit ignore entry.
repo-update and repo-type-update -h must substitute the placeholder with every IntegrityCheckerType, the same way create already does.
…after installing Mojito.
b477f14 to
5d674db
Compare
Document repository types for CLI usage
Summary
Repo types can now set integrity checkers from the CLI, the same way repositories already can.
repo-type-create --integrity-checkstores the checkers with the new type.repo-type-update --integrity-checkreplaces the whole set. Leave the flag off to keep the current checkers. Pass an empty value (-it "") to clear them.repo-type-viewprints each checker asextension:TYPE.repo-type-listshows how many checkers a type has. Add--verboseto print each one on its own line.--integrity-checklists everyIntegrityCheckerType, including types that were missing from the old hardcoded list.Test
1. Create a repository type with integrity checkers
2. List repository types
By default, the command displays the number of configured integrity checkers.
Example output:
Use
--verboseto display each checker separately.Example output:
3. View a repository type
Example output:
4. Update or clear integrity checkers
Pass an empty value to
--integrity-checkto remove all configured checkers.Verify that the checkers have been removed.
The output no longer contains an
Integrity checkersentry.5. Assign a repository type
Create a repository using the repository type.
Verify the assignment.
Example output:
6. Delete a repository type
A repository type cannot be deleted while it is assigned to one or more repositories.
Remove the repository type assignment.
Delete the repository type.