Allow Fly to build locally #1006
Conversation
shayneczyzewski
changed the title
| @@ -20,6 +20,10 @@ class FlyCommand extends Command { | |||
| .option('--initial-cluster-size <initialClusterSize>', 'flyctl postgres create option', '1') | |||
| .option('--volume-size <volumeSize>', 'flyctl postgres create option', '1'); | |||
| } | |||
| addLocalBuildOption(): this { | |||
| return this.option('--build-server-locally', 'force the server Docker container to build locally', false) | |||
There was a problem hiding this comment.
What happens otherwise? It always get's built remotely?
When I read "forces", it sounds like normally it might build locally or it might build remotely, but now we made sure it builds locally -> but is that correct?
We could mention default behaviour here if there is one.
There was a problem hiding this comment.
The current implementation defaults to building remotely, which is mentioned in the docs (I can mention here too). This flag forces local. The default behavior of flyctl deploy without any flags is to build locally if Docker was running, but otherwise build remotely. I am not a big fan of that for Wasp users to help support and debug build failures, etc., so was thinking it may benefit us to make it 100% deterministic, so we just default to remote but they can override and build locally if the need arises. Does that make sense?
There was a problem hiding this comment.
Actually, to probably better address your point, I can make it read something like build Docker containers locally instead of remotely or something like that?
| @@ -20,6 +20,10 @@ class FlyCommand extends Command { | |||
| .option('--initial-cluster-size <initialClusterSize>', 'flyctl postgres create option', '1') | |||
| .option('--volume-size <volumeSize>', 'flyctl postgres create option', '1'); | |||
| } | |||
| addLocalBuildOption(): this { | |||
| return this.option('--build-server-locally', 'force the server Docker container to build locally', false) | |||
| .option('--build-client-locally', 'force the client Docker container to build locally', false); | |||
There was a problem hiding this comment.
Just curious, how is it that we have two commands, and not just one --build-locally that would build everything locally? When might one want to build one thing locally and other thing not?
There was a problem hiding this comment.
Unsure, I can collapse it to one option for now and we can split if needed later on.
| } | ||
| } | ||
|
|
||
| async function deployServer(deploymentInfo: DeploymentInfo) { | ||
| async function deployServer(deploymentInfo: DeploymentInfo, buildServerLocally: boolean) { |
There was a problem hiding this comment.
Nit: I prefer prefer using objects for options vs. providing boolean params.
await deployServer(..., true)
vs.
await deployServer(..., { buildClientLocally: true })
Your code is clear to me because you are using it like this:
await deployClient(..., options.buildClientLocally);
but I'm just looking out for the future, and making this code easier to extend with additional options if we need to.
There was a problem hiding this comment.
Good point, can do 👍🏻
Description
Allows
wasp deploy flyusers to specify if server/client should be built locally (using their own Docker) by optionally passing--build-locallytolaunchand/ordeploy. Before, it would always just run on the Fly builder. This may not be ideal if the build process requires lots of resources/time, and they cannot use the Free Builder for it.It also fixes up the use of shared options and how those are expressed in the interfaces.
Select what type of change this PR introduces:
Update Waspc ChangeLog and version if needed
If you did a bug fix, new feature, or breaking change, that affects waspc, make sure you satisfy the following: