This repository was archived by the owner on Jul 9, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 375
fix: set func-related settings during build step #7723
Merged
Merged
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit
Hold shift + click to select a range
4eea8e9
set func-related settings during build step
benbrown 969a6c0
slight refactor to reduce duplicated code
benbrown e1269b5
add type
benbrown 8040e5d
Merge branch 'main' into benbrown/fixesfunctions
benbrown 15ae619
add typings in runtime plugin definition
benbrown 73427d7
Merge branch 'benbrown/fixesfunctions' of https://github.com/microsof…
benbrown 70200b6
set type of port to number
benbrown 18d4129
Merge branch 'main' into benbrown/fixesfunctions
benbrown 7281a9d
fix types, remove _ on variables that are used
benbrown a8212f5
Merge branch 'benbrown/fixesfunctions' of https://github.com/microsof…
benbrown File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -130,6 +130,19 @@ class LocalPublisher implements PublishPlugin<PublishConfig> { | |
|
|
||
| private publishAsync = async (botId: string, version: string, fullSettings: any, project: any, user) => { | ||
|
benbrown marked this conversation as resolved.
Outdated
|
||
| try { | ||
| let port; | ||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Needed to move the port selection up in the code a bit so that it can be passed to the build step. |
||
| if (LocalPublisher.runningBots[botId]) { | ||
| this.composer.log('Bot already running. Stopping bot...'); | ||
| // this may or may not be set based on the status of the bot | ||
| port = LocalPublisher.runningBots[botId].port; | ||
| await this.stopBot(botId); | ||
| } | ||
| if (!port) { | ||
| // Portfinder is the stablest amongst npm libraries for finding ports. https://github.com/http-party/node-portfinder/issues/61. It does not support supplying an array of ports to pick from as we can have a race conidtion when starting multiple bots at the same time. As a result, getting the max port number out of the range and starting the range from the max. | ||
| const maxPort = max(map(LocalPublisher.runningBots, 'port')) ?? 3979; | ||
| port = await portfinder.getPortPromise({ port: maxPort + 1, stopPort: 6000 }); | ||
| } | ||
|
|
||
| // if enableCustomRuntime is not true, initialize the runtime code in a tmp folder | ||
| // and export the content into that folder as well. | ||
| const runtime = this.composer.getRuntimeByProject(project); | ||
|
|
@@ -140,11 +153,11 @@ class LocalPublisher implements PublishPlugin<PublishConfig> { | |
| await this.saveContent(botId, version, project.dataDir, user); | ||
| } else if (project.settings.runtime.path && project.settings.runtime.command) { | ||
| const runtimePath = project.getRuntimePath(); | ||
| await runtime.build(runtimePath, project); | ||
| await runtime.build(runtimePath, project, fullSettings, port); | ||
| } else { | ||
| throw new Error('Custom runtime settings are incomplete. Please specify path and command.'); | ||
| } | ||
| await this.setBot(botId, version, fullSettings, project); | ||
| await this.setBot(botId, version, fullSettings, project, port); | ||
| } catch (error) { | ||
| await this.stopBot(botId); | ||
| this.setBotStatus(botId, { | ||
|
|
@@ -332,22 +345,9 @@ class LocalPublisher implements PublishPlugin<PublishConfig> { | |
| }; | ||
|
|
||
| // start bot in current version | ||
| private setBot = async (botId: string, version: string, settings: any, project: any) => { | ||
| private setBot = async (botId: string, version: string, settings: any, project: any, port: string) => { | ||
| // get port, and stop previous bot if exist | ||
| try { | ||
| let port; | ||
| if (LocalPublisher.runningBots[botId]) { | ||
| this.composer.log('Bot already running. Stopping bot...'); | ||
| // this may or may not be set based on the status of the bot | ||
| port = LocalPublisher.runningBots[botId].port; | ||
| await this.stopBot(botId); | ||
| } | ||
| if (!port) { | ||
| // Portfinder is the stablest amongst npm libraries for finding ports. https://github.com/http-party/node-portfinder/issues/61. It does not support supplying an array of ports to pick from as we can have a race conidtion when starting multiple bots at the same time. As a result, getting the max port number out of the range and starting the range from the max. | ||
| const maxPort = max(map(LocalPublisher.runningBots, 'port')) ?? 3979; | ||
| port = await portfinder.getPortPromise({ port: maxPort + 1, stopPort: 6000 }); | ||
| } | ||
|
|
||
| // if not using custom runtime, update assets in tmp older | ||
| if (!settings.runtime || settings.runtime.customRuntime !== true) { | ||
| this.composer.log('Updating bot assets'); | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
portbeing a string seems off. in which scenarios arefullSettingsandportprovided? Only functions?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.
I know port can be non-numeric in some cases. Would any be better?
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.
string|number is an option. But, in what case would port be a string?
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.
Windows app services listen on a named pipe which is expressed as a string. Unix sockets are also strings.
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.
@joshgummersall are those used in these scenarios? We're not firing up unix sockets or windows app services when running the bot locally are we?
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.
Doubtful, but if this code interacts with deployments at all it may be wise to keep that option open. If it's purely for local execution of bots, good chance you can limit it to numbers.
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.
It's all about the semantic of it:
port is a 16-bit unsigned integer, thus ranging from 0 to 65535, representation of it in our app should follow this semantic imho. When and if we get to calling external services such as things mentioned above, we can always map to the expected type.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 will be present for calls to build on any of the new runtimes. However since we haven't yet deprecated the old runtime support, not every instance has been updated.
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.
type of port updated to be a number