Uh oh!
There was an error while loading. Please reload this page.
feat: conclude manifest-sync experiment - #624
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@## main #624 +/- ##
==========================================
- Coverage 72.24% 72.23% -0.02%
==========================================
Files 238 238 Lines 20072 20021 -51 ==========================================
- Hits 14502 14463 -39 + Misses 4299 4291 -8 + Partials 1271 1267 -4 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej Thanks for getting this started. I'm requesting changes now because we still have a few edges to polish before this seems stable IMHO:
- Recommending a CLI-managed
manifest.jsonfile as the source of truth. We should removeget-manifestfrom our own hook implementations as part of this. I'm not meaning to deprecate it altogether, but change our preferred default. A frustrating experience appears often otherwise when remote changes aren't saved to file because of a custom hook implementation.
! No manifest.json found in project root — merged manifest was not written locally
- Missing flags in commands calling
manifest sync. The "run" command shows this and makes me want to revisit the option of #617 while still in experiment to use perhaps:
--manifest-source=project|remote
But "local" might be a more common term at the moment. Here's the error for reference:
$ lack run --app=A0BP2KFNZSQ | cat
App Manifest
Found 1 difference(s) between project and app settings
features.app_home.messages_tab_enabled
Project: false
App settings: true
Check /Users/eden.zimbelman/.slack/logs/slack-debug-20260808.log for error logs
The app manifest was not updated (app_manifest_update_error)
Suggestion
Run `lack manifest sync` interactively to resolve manifest differences, or pass --force to push the project manifest to app settings or --force-remote to pull app settings to project
$ lack run --app=A0BP2KFNZSQ --force-remote | cat
Check /Users/eden.zimbelman/.slack/logs/slack-debug-20260808.log for error logs
unknown flag: --force-remote
| originalSyncFunc := manifestSyncFunc | ||
| manifestSyncFunc = func(_ context.Context, _ *shared.ClientFactory, _ types.App, _ types.SlackAuth) (*manifestpkg.SyncResult, error) { | ||
| if tc.mockManifestSyncError != nil { | ||
| return nil, tc.mockManifestSyncError | ||
| } | ||
| return &manifestpkg.SyncResult{}, nil | ||
| } | ||
| t.Cleanup(func() { manifestSyncFunc = originalSyncFunc }) |
There was a problem hiding this comment.
🧪 thought: Significant logic is mocked with this pattern and I think we should avoid it to instead keep the manifestpkg.Sync function call inlined. Confidence in installations is meaningful to keep high!
🔬 ramble: I've opened an adjacent WIP PR #626 to collapse Install and InstallLocalApp functions into one because most logic is identical. I hope it improves the testing ergonomics here.
zimeg
commented
Aug 8, 2026
🗣️ Changelog for once it's stable. |
Summary
manifest-syncexperiment flag and enables two-way manifest sync by defaultslack manifest synccommandshouldUpdateManifestwith a call tomanifest.SyncTest plan
make lintpassesmake testpassesslack manifest syncworks without--experiment manifest-syncslack deploytriggers sync on manifest mismatch