-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Create wasp build start
command
#2796
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Create wasp build start
command
#2796
Conversation
…-runs-output-of-wasp-build
…-runs-output-of-wasp-build
Deploying wasp-docs-on-main with
|
Latest commit: |
6c8b47a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://b9119a34.wasp-docs-on-main.pages.dev |
Branch Preview URL: | https://1883-implement-wasp-build-st.wasp-docs-on-main.pages.dev |
…-runs-output-of-wasp-build
f269680
to
01ac717
Compare
Did a round of pair reviewing with Carlos on this one, will review it again once it is out of draft stage. |
@Martinsos one more round |
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.
Responded to comments @cprecioso !
…-command-that-runs-output-of-wasp-build
@infomiho ready, when we approve the approach I'll write docs |
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.
Left some suggestions
…-runs-output-of-wasp-build # Conflicts: # waspc/e2e-test/test-outputs/waspBuild-golden/waspBuild/.wasp/build/.waspchecksums # waspc/e2e-test/test-outputs/waspCompile-golden/waspCompile/.wasp/out/.waspchecksums # waspc/e2e-test/test-outputs/waspComplexTest-golden/waspComplexTest/.wasp/out/.waspchecksums # waspc/e2e-test/test-outputs/waspJob-golden/waspJob/.wasp/out/.waspchecksums # waspc/e2e-test/test-outputs/waspMigrate-golden/waspMigrate/.wasp/out/.waspchecksums
…-command-that-runs-output-of-wasp-build
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 left some suggestions
makeEnvironmentVariableParser prefix = | ||
Opt.strOption $ | ||
Opt.long (prefix <> "-env") | ||
<> Opt.short (head prefix) |
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.
Nitpick: maybe we drop the short option since it might cause some conflicts (now s
and c
are taken, who knows if in the future we want to add new long options e.g. secret-sauce
which would result in another s
option jk but you get my point)
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.
Yeah I know, but if we want to add multiple envs, wasp build start --server-env JWT_SECRET=my-secret --server-env GOOGLE_CLIENT_ID=my-client-id --server-env GOOGLE_CLIENT_SECRET=my-client-secret
etc is going to get old very quickly...
parseDotEnvFile :: Path' Abs (File ()) -> IO [EnvVar] | ||
parseDotEnvFile envFile = | ||
Dotenv.parseFile (fromAbsFile envFile) | ||
parseDotEnvFilePath :: FilePath -> IO [EnvVar] |
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.
We are losing type safety here e.g. FilePath
is much less defined than Path' Abs (File ())
. Can we avoid doing this and parse the user provided file path? For example, we assume it will be a relative file path which we can then combine with the Wasp project dir into an absolute file path?
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 spent some time fighting with SP but I don't think I was able to express what I wanted. The args can both be given as an absolute path, or as a relative path to cwd. How can I parse that with SP?
|
||
extraEnvParamsFromConfig = | ||
Config.serverEnvironmentVariables config | ||
>>= \envVar -> ["--env", envVar] |
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.
We are not parsing the user provided server env variables here. We are doing much more with the client env vars e.g. parsing them into EnvVar
and then nubing them. I'd like us to do the same here so we can be sure that we are not passing malformed input to the docker
command.
We coule maybeparse the env vars at the beginning into EnvVar
format which we can then show envVar
here. That would make sense, because we don't need a list of strings in our config, but a list of EnvVar
values.
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.
We are doing much more with the client env vars
Yep, because we need to construct the env for the client manually. but here in docker we can just pass both as arguments, so I opted to skip the complex processing (esp. since Docker will just do it again).
Especially, converting to EnvVar
((String, String)
), seems a bit pointless as I'm going to immediately convert them back to NAME=VALUE
(because that's how docker expects it); plus if I do that I don't allow the passthrough syntax from Docker (-e JUST_THE_NAME
).
Resolves #1883
Implements a new
wasp build start
command.It will take the output from
wasp build
, build the client, the server, and run them both.The logic is consistent with
wasp-app-runner
, except we don't run the SMTP server or the DB.Nothing is excessively surprising, except maybe the creation of a
JobExcept
type to make working with the jobs easier. This might later grow into a full orchestration utility if we want.