Conversation
|
|
| Port: strconv.Itoa(p), | ||
| Source: "app.yaml", |
There was a problem hiding this comment.
I am ok to remove the webview from the app.yaml definition.
Having this change, it is not possible to define a port in the app.yaml with webview behaviour.
It is only possible to set it from the brick definition.
There was a problem hiding this comment.
I agree. I think we have a bigger problem here.
- The
requires_displayin thebrick_config.yamlis at the root level. If you have multiple ports, it's not clear which one should be opened. - There is not way to specify the same concept "requires display" in the ports of the
app.yamlfile.
It would be nice to have the "port type" (which includes the concept of "requires display") for each port, and make it work the same both in brick_config.yaml and app.yaml.
Something like (see comments):
id: arduino:web_ui
name: WebUI - HTML
category: ui
# requires_display: webview # REMOVED, NOT SUPPORTED
ports:
- 7000: webview # port should be opened in a browser when the app is launched (and thus forwarded by App Lab)
- 8000: internal # port should not be opened or forwarded. used for brick-to-brick communication
- 9000 # no type: port will be forwarded by App Lab, but not opened in a browser| if strings.Contains(portStr, ":") { | ||
| parts := strings.Split(portStr, ":") | ||
| hostPort := parts[len(parts)-2] // Extract the host port (the one before the last colon) | ||
| index.Bricks[i].containerPorts = append(index.Bricks[i].containerPorts, hostPort) | ||
| } else { | ||
| index.Bricks[i].containerPorts = append(index.Bricks[i].containerPorts, portStr) | ||
| } | ||
| } |
There was a problem hiding this comment.
I would add a unit test for the port parsing
Currenlty we have this ${BIND_ADDRESS:-127.0.0.1}:1340:1337
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer f.Close() |
There was a problem hiding this comment.
having a defer f.Close() inside the loop causes many pending f.Close() at the same time, since the defer will be called at the end of Load().
We could extract a helper function to get the ports: getComposePorts(composeFilePath *paths.Path) ([]string, error)
6 participants
Motivation
Change description
Additional Notes
Reviewer checklist
main.