feat: add --manifest-source flag to run and deploy commands - #630
feat: add --manifest-source flag to run and deploy commands#630srtaalej wants to merge 12 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #630 +/- ##
==========================================
+ Coverage 78.19% 78.22% +0.03%
==========================================
Files 239 239
Lines 18149 18176 +27
==========================================
+ Hits 14192 14219 +27
Misses 3957 3957 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
srtaalej
added
enhancement
zimeg
left a comment
zimeg
left a comment
There was a problem hiding this comment.
@srtaalej I appreciate the most patient request for review 🙏 ✨ I'm requesting a few changes with hopes these comments move this in evermore stable directions. I call out:
- Removing
--force-remoteflag altogether alongside this change: I'd like to avoid multiple options unless follow up is planned to remove this too? - Favoring the existing terms and implementations of manifest source: Our configuration file has some logic we might reuse here!
If I can share more to these please let me know! I'm optimistic we include this in upcoming release 🚀 🔮
| style.CommandText("--manifest-source=project / --force"), | ||
| style.CommandText("--manifest-source=remote / --force-remote"), |
There was a problem hiding this comment.
🪓 question: Are we alright to replace the --force and --force-remote options altogether while the sync command is under experiment?
There was a problem hiding this comment.
yes i think thats a nice consolidation!
| ManifestSourceFlag string | ||
| LogstashHostResolved string |
There was a problem hiding this comment.
🧮 suggestion: Let's keep this in alphabetical order!
| const ( | ||
| ManifestSourceProject = "project" | ||
| ManifestSourceRemote = "remote" | ||
| ) |
There was a problem hiding this comment.
| const ( | |
| ManifestSourceProject = "project" | |
| ManifestSourceRemote = "remote" | |
| ) | |
| const ( | |
| ManifestSourceProject = "local" | |
| ManifestSourceRemote = "remote" | |
| ) |
🪬 suggestion(blocking): Earlier suggestion might've hinted at "project" terms but we should match existing configuration options I realize. Perhaps reusing logic from this package instead of validations here?
slack-cli/internal/config/manifest.go
Lines 20 to 23 in 73dde37
| if err := cmdutil.ValidateManifestSourceFlag(clients); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
| if err := cmdutil.ValidateManifestSourceFlag(clients); err != nil { | |
| return err | |
| } |
🪓 quibble: I'd favor this validation happening with the switch case in internal/manifest/sync.go to avoid duplicate checks in code, although I understand this might error earlier.
There was a problem hiding this comment.
we actually do need the check to happen in run and deploy as well otherwise invalid values in the flag will fall through
2 participants
Summary
--manifest-source=project|remoteflag toslack runandslack deploycommandsslack manifest syncto be run separately--manifest-sourceis set —projectauto-approves the overwrite,remoteskips it entirely--manifest-sourceinstead of--force/--force-remote(which are only available onmanifest sync)Closes #628
Test plan
make lintpassesmake testpassesslack run --manifest-source=projectpushes local manifest without promptingslack run --manifest-source=remotepulls app settings without promptingslack deploy --manifest-source=projectworks in non-TTY (CI) environmentsslack run --manifest-source=invalidreturns a clear validation errorslack run --manifest-source=remoteskips the "Overwrite manifest?" prompt on reinstall