add custom-container local start #1066
Merged
Conversation
This file contains 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
SquatsTonight
force-pushed
the
add-local-invoke-to-custom-container
branch
2 times, most recently
from
91b03aa
to
a00f08c
Compare
rsonghuster
reviewed
Jan 27, 2021
src/lib/docker-opts.js
Outdated
| return opts; | ||
| } | ||
|
|
||
| async function generateLocalStartOpts(runtime, name, mounts, cmd, debugPort, envs, dockerUser, imageName, debugIde, caPort=9000) { |
There was a problem hiding this comment.
这两个重载的函数逻辑大部分相同， 不能合成一个函数？
There was a problem hiding this comment.
generateCustomContainerLocalStartOpts 和 generateLocalStartOpts 这两个函数逻辑还是有所不一样的，如果合成一个函数，需要一些 if...else 去进行判断，代码可读性会下降，同时函数参数上会出现一些冗余，这些冗余会让这个函数在调用时的可理解性下降，因此这里是分为两个函数去写的。
rsonghuster
reviewed
Jan 27, 2021
src/lib/docker-opts.js
Outdated
| Mounts: mounts | ||
| } | ||
| }; | ||
| if (runtime === 'custom-container') { |
There was a problem hiding this comment.
custom-container 能不能定义成一个全局的 const 的变量， fun这个工程应该有 const 全局变量文件吧
rsonghuster
reviewed
Jan 27, 2021
src/lib/local/api-invoke.js
Outdated
| @@ -19,22 +20,16 @@ class ApiInvoke extends Invoke { | |||
| await super.init(); | |||
|
|
|||
| this.envs = await docker.generateDockerEnvs(this.baseDir, this.serviceName, this.serviceRes.Properties, this.functionName, this.functionProps, this.debugPort, null, this.nasConfig, true, this.debugIde, this.debugArgs); | |||
| this.cmd = docker.generateDockerCmd(this.functionProps, true); | |||
| if (this.runtime === 'custom-container') { | |||
There was a problem hiding this comment.
还是一个风格问题， 为什么搞两个函数， generateDockerCmd 和 generateDockerCmdOfCustomContainer
我觉得 generateDockerCmd 一个函数就好， 多加一个 runtime 的参数， custom-container 有不同的处理让 generateDockerCmd这个函数搞定就好
rsonghuster
reviewed
Jan 27, 2021
SquatsTonight
force-pushed
the
add-local-invoke-to-custom-container
branch
3 times, most recently
from
43aa79d
to
e7d03aa
Compare
SquatsTonight
force-pushed
the
add-local-invoke-to-custom-container
branch
3 times, most recently
from
cb9fda8
to
8b652b7
Compare
SquatsTonight
force-pushed
the
add-local-invoke-to-custom-container
branch
from
8b652b7
to
49be133
Compare
rsonghuster
approved these changes
Feb 4, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
增加 custom-container 的 local invoke/start；
custom-container 本地调试启的容器，不更改 PATH 环境变量，该环境变量由用户自定义。