-
Notifications
You must be signed in to change notification settings - Fork 24
fix: gather manifest info from the configured source after parsing flags #113
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #113 +/- ##
=======================================
Coverage 63.45% 63.45%
=======================================
Files 212 212
Lines 22336 22331 -5
=======================================
- Hits 14173 14171 -2
Misses 7071 7071
+ Partials 1092 1089 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zimeg
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📝 A note for the wonderful reviewers reviewing!
| return "", slackerror.New(slackerror.ErrInvalidManifestSource). | ||
| WithMessage(`Cannot get manifest info from the "%s" source`, config.ManifestSourceRemote). | ||
| WithRemediation("%s", strings.Join([]string{ | ||
| fmt.Sprintf("Find the current manifest on app settings: %s", style.LinkText("https://api.slack.com/apps")), | ||
| fmt.Sprintf("Set \"manifest.source\" to \"%s\" in \"%s\" to continue", config.ManifestSourceLocal, filepath.Join(".slack", "config.json")), | ||
| fmt.Sprintf("Read about manifest sourcing with %s", style.Commandf("manifest info --help", false)), | ||
| }, "\n")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📣 These error details are never reached due to the err from GetManifestSource above - opting to fallback to the default message if this errors!
mwbrooks
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Really really really nice fix @zimeg 🌮 🎉
| } | ||
| return config.ManifestSourceRemote, nil | ||
| } | ||
| if clients.Config.WithExperimentOn(experiment.BoltFrameworks) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🪓 Nice to slowly remove the old BoltFrameworks experiment!
|
@mwbrooks Thanks so much! 👾✨ I'm also glad to find we're cleaning parts of the code up with improvements otherwise. Since the |
Summary
This PR uses project configurations to determine the default manifest source of the manifest
infocommand instead of defaulting to "local" because that might error for projects using a remote manifest!🏁 Also changed is the order of parsing for this value, preferring flags before file configurations.
🧪 Reference of the
boltexperiment are also removed from this section of the code as part of these changes.Reviewers
Using the changes with the
bolt-installexperiment, run these commands:Requirements