-
Notifications
You must be signed in to change notification settings - Fork 24
feat(bolt-install): support using 'run' command with remote manifests #111
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 #111 +/- ##
==========================================
- Coverage 63.46% 63.45% -0.01%
==========================================
Files 212 212
Lines 22308 22335 +27
==========================================
+ Hits 14157 14172 +15
- Misses 7062 7068 +6
- Partials 1089 1095 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
🪄 A few comments for the kind reviewers!
| if !clients.Config.WithExperimentOn(experiment.BoltInstall) { | ||
| if !manifestUpdates && !manifestCreates { | ||
| return app, api.DeveloperAppInstallResult{}, "", nil | ||
| } |
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.
note: InstallLocalApp now continues even with there are no manifest updates or creates. This is because it must still install the app in order to get the tokens.
| // When the BoltInstall experiment is enabled, apps can always be created with any manifest source. | ||
| if clients.Config.WithExperimentOn(experiment.BoltInstall) { | ||
| return app.AppID == "", nil | ||
| } |
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.
note: shouldCreateManifest now always returns true when the experiment is turned on because both local and remote manifest sources can be created.
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.
@mwbrooks This is so good! 🧠 ✨
As later follow up we might want to wrap errors from .GetManifestLocal to suggest linking an created on app on app settings if the get-manifest hook errors for some reason and the manifest source is remote.
No blocker for this PR since IMO error handling is another discussion altogether!
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.
Edit: This might be best to note for the conclusion of this experiment to let us streamline some of the logic above and make this case more clear 🤔
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.
This is a really great suggestion! I think it would help developers move forward who don't have a local manifest file. I've made a note to follow-up on this error suggestion. ✏️
| isManifestSourceLocal := manifestSource.Equals(config.ManifestSourceLocal) | ||
| isBoltInstallEnabled := clients.Config.WithExperimentOn(experiment.BoltInstall) | ||
| if isManifestSourceLocal || isBoltInstallEnabled { |
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.
note: Updated the app select to display "create a new app" for both local and remote manifest sources. This means the slack install now displays "create a new app" for remote projects, even though it doesn't work. However, I think this is acceptable because is currently returns an error and the update continues to return an error. We'll follow-up this PR with support for slack install.
| if !strings.HasSuffix(name, LocalRunNameTag) { | ||
| name = name + " " + LocalRunNameTag | ||
| } | ||
| return name |
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.
note: Fixed a bug where (local) was appended multiple times.
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.
LGTM! The test cases are quite precise and review instructions were helpful 🙏 ✨
All is working well for me but I left a note on perhaps enhanced error logging and I also noticed that updating the manifest.json file while the app is running - despite having a remote source - causes a reinstallation to happen.
AFAICT this is unrelated to these changes, but I'm starting to think about how these manifest sources can be as one 👁️🗨️
No blockers and a huge unlock for the quickstart experience! 🔏
| // When the BoltInstall experiment is enabled, apps can always be created with any manifest source. | ||
| if clients.Config.WithExperimentOn(experiment.BoltInstall) { | ||
| return app.AppID == "", nil | ||
| } |
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.
@mwbrooks This is so good! 🧠 ✨
As later follow up we might want to wrap errors from .GetManifestLocal to suggest linking an created on app on app settings if the get-manifest hook errors for some reason and the manifest source is remote.
No blocker for this PR since IMO error handling is another discussion altogether!
| // When the BoltInstall experiment is enabled, apps can always be created with any manifest source. | ||
| if clients.Config.WithExperimentOn(experiment.BoltInstall) { | ||
| return app.AppID == "", nil | ||
| } |
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.
Edit: This might be best to note for the conclusion of this experiment to let us streamline some of the logic above and make this case more clear 🤔
internal/pkg/apps/install.go
Outdated
| // When the BoltInstall experiment is enabled, we need to get the manifest from the local file | ||
| // if the manifest source is local or if we are creating a new app. Otherwise, we get the manifest | ||
| // from the app settings. |
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.
@mwbrooks Small suggestion - it might be nice to note why we're collecting a remote manifest here? 📚
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.
| "create a new run on slack app with a local function runtime using expected rosi defaults": { | ||
| isExperimental: false, | ||
| mockApp: types.App{}, | ||
| "create and install a new ROSI app with a local function runtime using expected rosi defaults when the BoltInstall experiment is disabled": { |
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.
@mwbrooks Thanks for updating these test titles with the BoltInstall experiment status - huge help in later refactors 🪓
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.
@zimeg Thanks! It added some noise to the PR Review, but keeping the wording consistent will hopefully make removing the experiment a little easier.
| expectedCreate: false, | ||
| expectedInstallState: types.InstallSuccess, | ||
| expectedUpdate: false, |
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.
@mwbrooks Wonderful 👾 ✨
…le or app settings
|
Thanks for the quick and thorough review @zimeg! 🙇🏻
Oh, this is a really good catch! 🏒 🥅 I imagine the ideal behaviour would be something along the lines of a warning message when the local manifest file is changed and the source is remote. This would help reduce some confusion. I think we could do something similar at during any command that detects the local file has changed (cached hash). I'll make a note about this as a future enhancement. 💎 |
@mwbrooks 🧠 ✨ Wow this is so much better than a silent and confusing success! Thanks much for keeping note and making changes all around. |
CHANGELOG
Summary
This pull request updates the
slack runcommand to create and install new & existing apps that use app settings as the source of truth (remote manifest).Preview
slack runcreates a new Bolt App with app settings as the source of truth2025-05-29-run-new-bolt-app.mov
slack runuses an existing Bolt App with app settings as the source of truth2025-05-29-run-existing-bolt-app.mov
Reviewer
Requirements