feat(config-cli): Provide a cli for creating temporary server and dump screenshots. #2236
Conversation
packages/config-cli/README.md
Outdated
| Create a temporary server from a config bundle file and dump the screenshots | ||
|
|
||
| ```bash | ||
| ./bin/bmc.js screenshot-server - config s3://..../config.json.gz |
There was a problem hiding this comment.
I think this should just be a argument to screenshot
eg
./bin/bmc.js screenshot --config s3://.../config.json.gz
There was a problem hiding this comment.
Might be good to separate to two different cli? I think this will make the cli logic not too complex.
There was a problem hiding this comment.
should be pretty simiple logic ?
something like
if (this.config.value)
const host = await startServer()
return await screenshot(host);
}
return await screenshot(this.host.value)
they are both doing the same thing, screenshotting some host, so to me they are the same command
| const port = 5000; | ||
| const ServerUrl = `http://localhost:${port}`; | ||
| const BasemapsServer = createServer(logger); | ||
| BasemapsServer.listen(port, '0.0.0.0', () => { |
There was a problem hiding this comment.
this is async you should really await for the server to start before starting chrome.
granted chrome startup is going to be way slower than the server but it could create issues if the server startup takes too long
There was a problem hiding this comment.
createServer() is not async function? Do you want me to update to async?
There was a problem hiding this comment.
something like this
await new Promise(resolve => BaseampsServer.listen(port, '0.0.0.0', () => resolve()))
| Config.setConfigProvider(mem); | ||
|
|
||
| // Create a basemaps server. | ||
| const port = 5000; |
There was a problem hiding this comment.
this port should be a high random number, is there a npm package to find a unused port?
There was a problem hiding this comment.
Using random free port from 10000 to 11000 now.
|
|
||
| async getTileSetId(tileSetId: string, tag: string): Promise<string> { | ||
| if (tag === 'production') return tileSetId; | ||
| async function getTileSetId(tileSetId: string, tag: string): Promise<string> { |
There was a problem hiding this comment.
I think we can remove these now.
if we are pointing at a bundled config there is not @tag to worry about any more!
There was a problem hiding this comment.
Pr tag removed. We can just point a lambda function url as host to take screenshot for linz/basemaps-config.
| BasemapsServer = createServer(logger); | ||
|
|
||
| if (BasemapsServer == null) throw new Error('Failed to Create server with the config File'); | ||
| BasemapsServer.listen(port, '0.0.0.0', () => { |
There was a problem hiding this comment.
this is not waiting for this to finish loading.
There was a problem hiding this comment.
Sorry, I forget this. Just fixed.
packages/config-cli/README.md
Outdated
| Dump the screenshots with config file | ||
|
|
||
| ```bash | ||
| ./bin/bmc.js screenshot - config s3://..../config.json.gz |
| if (config != null) { | ||
| const port = await getPort({ port: portNumbers(10000, 11000) }); | ||
| host = `http://localhost:${port}`; | ||
| const server = createServer(logger); |
There was a problem hiding this comment.
NIT: why create a new var if we could just use BasemapsServer
another option would be const BasemapsServer = this.startServer(config) which returns null | Server
There was a problem hiding this comment.
Not sure why if I pass the BasemapsServer into the await new Promise<void>() if always return type checking errors to say that BasemapsServer could be undefined even I have checked not null before. So I create a new var to pass it after.
There was a problem hiding this comment.
yeah if you did this as a function that returned Promise<Server | null> could be optimised out.
|
|
||
| // Start server | ||
| const server = createServer(logger); | ||
| server.listen(port, '0.0.0.0', () => { |
There was a problem hiding this comment.
Sorry, I copy from the wrong place. Just fixed.
|
|
||
| // Start server | ||
| const server = createServer(logger); | ||
| await new Promise<void>((resolve) => |
There was a problem hiding this comment.
return new Promise<FastifyInstance>(r => {
r(server);
});
| if (style) return styleIdTagId; | ||
| return styleId; | ||
| } | ||
| async function startServer(host: string, port: number, config: string, logger: LogType): Promise<FastifyInstance> { |
There was a problem hiding this comment.
why is host passed in just to log it?
could generate host inside the function or log outside the function
|
|
||
| let BasemapsServer: FastifyInstance | undefined = undefined; | ||
| if (config != null) { | ||
| const port = await getPort({ port: portNumbers(10000, 11000) }); |
No description provided.