feat(app): add settings support for app registration - #216
Conversation
There was a problem hiding this comment.
Pull request overview
Adds first-class support for application “settings” registrations in the CLI by extending godaddy.toml ([[settings]] placement + typed settings-form-v1 presentation), wiring settings into gddy platform app add settings and the release payload, and documenting the workflow.
Changes:
- Introduces new config models + structural validation for
[[settings]]placement metadata andsettings-form-v1presentation. - Adds
gddy platform app add settingsand includessettingsingddy platform app releasepayload mapping (with presentation required at release-time). - Adds a build/verify helper script and end-user documentation for authoring settings.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| rust/src/config/settings.rs | Defines SettingConfig/SettingIcon and validates placement metadata (slug, entryPath, capabilities, icon library, overlap). |
| rust/src/config/settings_form.rs | Defines typed settings-form-v1 presentation model plus structural validation (keys/options/uniqueness). |
| rust/src/config/mod.rs | Wires settings: Vec<SettingConfig> into Config + Config::validate(), and updates tests/config literals. |
| rust/src/application/commands/release.rs | Maps config settings into the createRelease.settings payload and enforces presentation required at release-time. |
| rust/src/application/commands/add.rs | Adds gddy platform app add settings to write placement metadata into godaddy.toml. |
| rust/src/application/commands/schemas.rs | Adds ConfigSetting output schema for the new add settings command. |
| rust/src/application/commands/init.rs | Ensures generated config includes settings: vec![]. |
| rust/scripts/build-and-verify.sh | Adds a helper script to run local verification + a debug build smoke run. |
| docs/application-settings.md | Documents the settings workflow and the settings-form-v1 TOML authoring shape. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
rust/src/config/settings_form.rs:265
- Nested list-group fields are only validated individually, so sibling fields may reuse the same key or reuse the reserved
idField. Both cases produce ambiguous object properties and contradict the documented guarantees that field keys are unique andidFieldcannot be editable. Track keys within each item and reject both collisions.
for (k, inner) in item.fields.iter().enumerate() {
validate_field(inner, errors, &format!("{path}.item.fields[{k}]"));
}
rust/scripts/build-and-verify.sh:20
- This verification script omits the repository's required
cargo testcheck, so it can report success despite test failures. Add the test suite to the verification sequence.
step "cargo clippy -- -D warnings"
cargo clippy -- -D warnings
rust/src/application/client.rs:191
- The release response now includes
settings, butApplicationReleasestill omits that field. Consequentlyrelease --helpandrelease --schemado not advertise the newly emitted JSON member, contrary to the synchronization requirement inrust/src/output_schema.rs:15-17. Addsettingsas an optional[]objectfield to the release output schema.
"query": "mutation CreateRelease($input: MutationCreateReleaseInput!) { createRelease(input: $input) { id version description createdAt uiExtensions { id name handle type source target } settings { id groupSlug appSettingSlug entryPath capabilities order title } } }",
|
@nmolham-godaddy thanks for driving this end-to-end — I validated the implementation against the live App Registry settings schema, the #131 registration plan, and DEVEX-1021. I found two blockers before this can merge:
Follow-up: the docs say local validation enforces key uniqueness, but nested The core settings mapping and targeted GraphQL test otherwise align with App Registry. |
|
The API mapping here looks right, but we should support both presentation authoring modes before merging: inline TOML and a referenced JSON file. Suggested manifest shape: [[settings]]
group = "tax-center"
slug = "manual-tax"
entryPath = "/settings/manual-tax"
presentationFile = "fixtures/manual-tax-registry-presentation.json"The referenced JSON should contain the complete API presentation object ( Expected behavior:
This is especially useful for the existing GPA examples, which already keep substantial registry presentations in JSON fixtures; requiring inline-only TOML would create a second source of truth. |
There was a problem hiding this comment.
It'd be better if this was embedded in a guide so that LLMs can discover these instructions without needing to do a web search. I think the whole gddy platform module probably needs a holistic guide with application settings included (or I guess multiple guides is also fine).
There was a problem hiding this comment.
I agree, sounds like a good idea, I will do my best effort to implement it
There was a problem hiding this comment.
b298b7a what do you think? CC @wcole1-godaddy
| label = "Canada" | ||
| value = "CA" | ||
| ``` | ||
| - Optional: `defaultValue` must match one option's `value`. |
There was a problem hiding this comment.
Looks like the Markdown parser hit a snag starting on this line. You probably need to fix indentation or some other kind of white space.
There was a problem hiding this comment.
I can see rendering fine 🤔 what do you see when you open the file ?
https://github.com/nmolham-godaddy/gddy-cli/blob/2f77e886a0e5eb551747bd645d4a4b962cc03cbd/docs/application-settings.md#presentation-shape
There was a problem hiding this comment.
I mean in the GitHub UI diff view. But if it renders fine, that's ok.
| "Register the placement metadata for an application-settings \ | ||
| capability in the godaddy.toml manifest in the current directory. \ | ||
| This command only writes group/slug/entryPath/order/capabilities/icon \ | ||
| — it cannot author the settings-form-v1 form itself. After running \ |
There was a problem hiding this comment.
Is the meaning of "settings-form-v1" something that's clear to users?
There was a problem hiding this comment.
initially yes, but according to Wes's comment here, I should also add presentation authoring modes in this PR, so this description will change
| error = %e, | ||
| path = %config_path.display(), | ||
| "failed to read config; releasing with empty actions, subscriptions, and uiExtensions" | ||
| "failed to read config; releasing with empty actions, subscriptions, uiExtensions, and settings" |
There was a problem hiding this comment.
I know this isn't new behavior, but if someone doesn't notice this warning, it could cause a lot of frustration, if they're releasing but it doesn't reflect their config. As a user, I'd prefer the release failed instead.
(non-blocking)
There was a problem hiding this comment.
Agreed, fixed c4c0115 what do you think?
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 20 changed files in this pull request and generated no new comments.
Suppressed comments (8)
rust/src/config/settings_form.rs:249
- Because both discriminators are optional and only checked when present, a JSON file that omits
typeand/orschemaVersionis accepted and silently normalized bysetting_entry. The documentedpresentationFilecontract requires the complete API object, so missing discriminators should fail just like incorrect ones.
if let Some(t) = &doc.r#type
&& t != "form"
rust/src/platform/guides/platform-overview.md:26
config validatedoes not check every rule the API enforces: the settings guide explicitly leaves bounds/default consistency and nesting depth to release-time server validation. This overstatement can lead users to treat a successful local check as proof that release will succeed.
Run `gddy platform app config validate` any time to check the manifest against every rule the API would otherwise enforce (required fields, URL/UUID/semver shapes, settings placement rules) without a network call — it reports every violation found, not just the first.
rust/src/config/settings_form.rs:207
- An empty
sectionsarray, or a section with an emptyfieldsarray, currently passes local validation because these loops simply do nothing. Both user guides define the form as one or more sections with one or more fields, soconfig validatecan report success for a presentation that will be rejected at release time. Validate both cardinalities here.
This issue also appears on line 248 of the same file.
let mut seen_section_keys = HashSet::new();
let mut seen_top_level_field_keys = HashSet::new();
for (i, section) in presentation.sections.iter().enumerate() {
let section_path = format!("{path}.sections[{i}]");
rust/src/config/settings_form.rs:296
- Nested
list-groupfields are recursively shape-checked but never uniqueness-checked. Two item fields with the same key therefore passconfig validate, despite the documented rule that field keys are unique, and would map to the same property in each saved item. Track keys within each item just as top-level field keys are tracked.
for (k, inner) in item.fields.iter().enumerate() {
validate_field(inner, errors, &format!("{path}.item.fields[{k}]"));
}
rust/src/application/client.rs:191
- The real GraphQL response cannot contain the setting fields omitted from this selection (
presentation,description, icon fields, andmetadata), even though the new smoke test asserts thatreleaseechoes them. That test passes only because the mock copies the entire request input without honoring the selection set. Select the fields the command promises to return, or change the smoke contract to test only selected fields.
"query": "mutation CreateRelease($input: MutationCreateReleaseInput!) { createRelease(input: $input) { id version description createdAt uiExtensions { id name handle type source target } settings { id groupSlug appSettingSlug entryPath capabilities order title } } }",
rust/src/platform/guides/platform-overview.md:24
- This says the form must be hand-authored, but
add settingsnow supports--presentation-file. Mention both supported workflows so the lifecycle overview does not contradict the dedicated settings guide.
This issue also appears on line 26 of the same file.
- `add settings --group <group> --slug <slug> --entry-path <path> ...` — placement metadata for a merchant-facing settings form. This only writes placement fields (group/slug/entryPath/order/capabilities/icon); the form itself (`[settings.presentation]`) has to be hand-authored in `godaddy.toml` afterward. See the `platform-settings` guide (`gddy guide platform-settings`) for the full presentation shape.
rust/src/platform/guides/platform-overview.md:34
- A setting backed by
presentationFileis valid without a[settings.presentation]block, so this release description incorrectly says that such an entry fails. State that release fails only when both presentation sources are absent.
Resends every action, subscription, UI extension, and settings entry currently in `godaddy.toml` as one versioned release — omitting an entry from the manifest doesn't archive it globally, but a store enabled against a *newer* release won't have it. A settings entry with no `[settings.presentation]` block fails the release with a `VALIDATION_ERROR`; a manifest that fails to parse or validate fails the release outright (only a genuinely missing manifest falls back to an empty release). Version must be semver.
rust/src/application/commands/add.rs:187
- The long help omits the newly supported
--presentation-filepath and says this command writes only placement fields, although it also writespresentationFile. This directs users exclusively to hand-edit TOML despite the available flag.
This command only writes group/slug/entryPath/order/capabilities/icon \
— it cannot author the settings-form-v1 form itself. After running \
it, hand-add a [settings.presentation] block (sections and fields) \
to the written entry; `gddy platform app release` rejects a \
settings entry with no presentation.",
There was a problem hiding this comment.
Well done on the guide
jpage-godaddy
left a comment
There was a problem hiding this comment.
Short of help text tweaks, the rest looks good to me.
|
@jpage-godaddy all requested changes implemented |
Summary
[[settings]]support togodaddy.tomland wire it throughgddy platform app add settings/gddy platform app release— closes the gap whereapp-registry-api'screateRelease.settingscapability had no CLI support at all (config, add command, and release payload all silently dropped it).gddy platform app add settingscommand writes placement metadata only (group/slug/entryPath/order/capabilities/icon) — matches the flag shape ofadd action/add subscription/add extension.settings-form-v1presentation model (text/textarea/number/boolean/select/multi-select/list-group, recursive nesting) authored inline ingodaddy.toml— no JSON file, no opaque passthrough.presentationis hand-added afteradd settings;releaserejects a settings entry with nopresentationinstead ofConfig::validate(), so a placement-only entry still works fine for every other command.options) — bounds/default-consistency/nesting-depth stay server-validated, consistent with how the rest ofConfig::validate()already treats every other section.rust/scripts/build-and-verify.sh— check/clippy/fmt/module-size, then build and rungddy(args forwarded, defaults to--help).docs/application-settings.md— end-user guide: workflow, full field-type reference, worked nestedlist-groupexample, gotchas (no release inheritance, no auto-backfill to already-enabled stores).Affects: https://github.com/gdcorp-commerce/app-registry-api/pull/131
Changes
rust/src/config/settings_form.rs(new) —SettingsFormV1Presentation/Section/Fieldenum/ListGroupItem/ChoiceOption/SelectValue, structural validation.rust/src/config/settings.rs(new) —SettingConfig/SettingIcon, slug/entryPath/capability/icon-library validation, entry-path-overlap check.rust/src/config/mod.rs—settings: Vec<SettingConfig>field, wired intoConfig::validate().rust/src/application/commands/release.rs—setting_entry/build_settingsmap config toApplicationSettingCreateInput, enforcepresentationrequired here (not inConfig::validate()).rust/src/application/commands/add.rs—add settingscommand.rust/src/application/commands/schemas.rs—ConfigSettingoutput schema.rust/src/application/commands/init.rs— add missingsettings: vec![]to aConfigliteral.rust/scripts/build-and-verify.sh(new).docs/application-settings.md(new).QA
cargo checkcargo clippy -- -D warningscargo test(19 new tests: config validation,settings_formstructural checks + TOML round-trip,release::setting_entrymapping incl. missing-presentation error path)cargo fmt --check./rust/scripts/check-module-size.shadd settingsagainst a scratchgodaddy.toml, hand-added[settings.presentation](incl. nestedlist-groupwith multipleoptions), confirmed round-trip throughread_config/validate/write_configpreservesdefaultValueand every nested field; confirmed--icon-name/--icon-librarypairing and entry-path-overlap rejections fire before any write.After merge
docs/GPA-SETTINGS-REGISTRATION.mdin https://github.com/gdcorp-commerce/app-registry-api/pull/131 if applicable