fix: disable upgrade notification for the manifest info command#548
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #548 +/- ##
==========================================
- Coverage 71.66% 71.63% -0.03%
==========================================
Files 225 225
Lines 19066 19074 +8
==========================================
+ Hits 13663 13664 +1
- Misses 4199 4205 +6
- Partials 1204 1205 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ignoredCommands := []string{"_fingerprint", "api", "version"} | ||
| osStr := os.Args[0:] | ||
| if len(osStr) < 2 { | ||
| ignoredCommands := []string{"_fingerprint", "api", "manifest info", "version"} |
There was a problem hiding this comment.
| ignoredCommands := []string{"_fingerprint", "api", "manifest info", "version"} | |
| ignoredCommands := []string{"_fingerprint", "api", "manifest", "manifest info", "version"} |
🪬 question: Is the shortened command automatic? I prefer extended commands often in scripts but am unsure if this should be covered too.
There was a problem hiding this comment.
brutal, I forgot we offer slack manifest as a short-form version of slack manifest info. We should probably deprecate that now that we have more manifest commands - normally running the top-level parent command would show the help info about the sub-commands.
answer: No, this matches against os.Args so it's the raw commands typed by the user. So aliases are not captured, either.
Let's add manifest here. It's safer because it captures manifest info but will also capture all other manifest commands. We'd require a re-write after Cobra parsing to capture aliases properly, but today the update goroutine is started before parallel to the Cobra parsing (not inside Cobra).
| }, | ||
| "manifest validate command": { | ||
| command: "manifest validate", | ||
| expected: false, |
There was a problem hiding this comment.
👁️🗨️ praise: Thanks! This makes me think much about tests as spec more than unit.
The top-level `manifest` command is an alias that runs `manifest info`, so it should also be ignored for upgrade notification checks. Co-Authored-By: Eden Zimbelman <eden.zimbelman@salesforce.com>
Changelog
Summary
This pull request adds
manifest infoto the list of commands that suppress update notifications. Themanifest infocommand outputs structured data consumed by other tools, so injecting an upgrade banner would corrupt the output.Also extends
isIgnoredCommand()to support multi-word subcommands (e.g.,manifest infois ignored butmanifest validateis not).Preview
N/A
Testing
go build -ldflags="-X 'github.com/slackapi/slack-cli/internal/version.Version=v1.0.0'" -o bin/slacklastUpdateCheckedAttimestamp from~/.slack/config.jsonso the check isn't skipped due to recency../bin/slack create my-app/ cd my-app/slack manifest infodoes NOT show an upgrade notification:./bin/slack manifest info --source localRequirements