feat: add two-way manifest sync between project and app settings#543
Draft
mwbrooks wants to merge 15 commits into
Draft
feat: add two-way manifest sync between project and app settings#543mwbrooks wants to merge 15 commits into
mwbrooks wants to merge 15 commits into
Conversation
When the manifest-sync experiment is enabled, the CLI detects per-field differences between the local manifest.json and the remote app settings, lets the user resolve them interactively, and writes the merged result to both sides. This replaces the binary "overwrite?" prompt when divergence is detected during install/deploy. Adds `slack manifest sync` command and `slack sync` alias. Gated behind --experiment=manifest-sync.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #543 +/- ##
==========================================
- Coverage 71.66% 71.05% -0.62%
==========================================
Files 226 233 +7
Lines 19176 19673 +497
==========================================
+ Hits 13743 13978 +235
- Misses 4222 4454 +232
- Partials 1211 1241 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2 tasks
Manifest sync would write to manifest.json regardless of the project's configured manifest source. Projects with ManifestSourceRemote treat app settings as the source of truth, so writing locally creates divergence. Refuse early with a clear remediation pointing at slack app settings.
Hides the slack manifest sync subcommand and the slack sync alias from --help, and refuses execution unless the manifest-sync experiment is enabled. Adds ErrExperimentRequired so the user gets a clear remediation pointing at the --experiment flag.
setNestedValue used an unchecked type assertion that panicked when flattened paths disagreed about whether a node is a scalar or an object (e.g. "a.b" alongside "a.b.c"). Replace the assertion with the comma-ok form and propagate ErrInvalidManifest. Also catch the mirror case where a leaf would silently overwrite an existing nested object.
afero.WriteFile truncates the destination before writing, so an interrupted write could leave manifest.json empty or half-written right after the API had already been updated. Switch to a sibling temp file plus rename so the destination is only ever the old contents or the new contents.
After Sync's UpdateApp succeeds, the cached manifest hash still reflected the pre-sync upstream, so a follow-up install or deploy would treat the merged result as drift and prompt the user to sync again. Persist the new hash right after the API push so the cache matches the current upstream state.
The non-TTY remediation pointed users at --force, but the flag was ignored. --force is a global persistent flag, so the message was quietly misleading. Treat --force as MergeAllLocal so CI flows can push their project manifest to app settings non-interactively.
marshalPreservingOrder discarded errors from json.Marshal and json.MarshalIndent with _, so a marshal failure on a malformed key or value would write malformed JSON like '" : "' to disk and return success. Propagate the errors so the user sees a real failure and the file is not overwritten.
marshalPreservingOrder silently fell back to default key ordering when the original file could not be parsed (BOM, trailing comma, malformed JSON), so users had no idea their carefully-ordered manifest had been reshuffled. Return a fellBack signal and surface it on WriteBackResult.Warning so the user is informed.
…nequality valuesEqual returned false when json.Marshal failed, so equal values with a marshal hiccup got flagged as a phantom diff that the user was then prompted to resolve. Surface the error up through Diff so the caller sees a real failure instead of fabricated differences.
Manifest function and workflow IDs may contain literal dots (e.g. "slack.users.lookup"). The original flatten/splitPath used dot as both delimiter and a valid key character, so a key like that round- tripped to a 4-level nested map and silently corrupted the merge. Backslash-escape dots and backslashes in path segments and honor the escape in splitPath.
Required by the license_check workflow.
internal/pkg is a generic bucket the project is migrating away from. Move the existing internal/pkg/manifest package (validate plus the new sync/diff/merge/flatten/writeback/display logic) up to internal/manifest and update its three importers.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changelog
Added
slack manifest synccommand (andslack syncalias) that detects per-field differences between the local manifest.json and remote app settings, prompts users to resolve conflicts, and writes the merged result to both sides. Gated behind--experiment=manifest-sync.Summary
This pull request adds two-way manifest sync so developers can resolve differences between their local manifest.json and the remote app settings on api.slack.com.
Today, when
manifest.source=local, the local manifest silently overwrites the API on next install — discarding changes made on the web. This feature replaces the binary "Overwrite?" prompt with a per-field resolution flow when divergence is detected.Key additions:
internal/pkg/manifest/— flatten, diff, merge, display, writeback, and sync orchestratorcmd/manifest/sync.go— standaloneslack manifest synccommandslack syncalias incmd/root.gomanifest-syncexperiment flag for gated rolloutshouldUpdateManifest()in the install flowPreview
Testing
make buildmanifest.json, install an app:./bin/slack install -e manifest-sync./bin/slack manifest sync -e manifest-sync— should report "in sync"./bin/slack manifest sync -e manifest-sync— should show the difference, choose "Use all app settings values", verify manifest.json updated./bin/slack manifest sync -e manifest-sync— choose "Use all project values", verify app settings updatedgit diff manifest.jsonshould only show value changes, not key reordering./bin/slack install -e manifest-sync— sync flow triggers instead of old "Overwrite?" prompt./bin/slack install -e manifest-sync --force— old behavior, local overwrites remote./bin/slack installwith a remote change — old "Overwrite?" prompt (unchanged behavior)Notes
set-manifesthook for code-generated manifests (e.g., manifest.ts) is not included in this initial implementation. For those projects, the merged manifest is pushed to the API only, with a warning that the local project was not updated.--force-remoteflag (remote overwrites local) is a follow-up item.Requirements