fix(render): reduce memory and handle server file #10055
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
| path.join(rwjsPaths.base, 'node_modules/.bin/prisma'), | ||
| ['migrate', 'deploy', '--schema', `"${rwjsPaths.api.dbSchema}"`], | ||
| 'node_modules/.bin/prisma', | ||
| ['migrate', 'deploy', '--schema', rwjsPaths.api.dbSchema], |
There was a problem hiding this comment.
Did this (the rwjsPaths.api.dbSchema) need to be explicitly quoted to support things like spaces in paths? I can't remember if execa handles that for you or not - I should look it up.
|
not sure but that seems likely. if we add back the quotes, we have to add back the shell: true config to execa. it just wasn’t working with those extra quotes and without the shell option. not sure exactly why again. but adding back shell probably doesn’t incur that much memory |
Josh-Walker-GM
approved these changes
Feb 22, 2024
|
I feel like escaping the args passed in is something execa should be doing so I would vote not to undo your change here. Unless I can find some execa docs stating otherwise |
jtoar
changed the title
|
@Josh-Walker-GM I couldn't be sure if the quotes really mattered since I didn't test a scenario for it so reverting on that for now which means adding |
jtoar
added a commit
that referenced
this pull request
Feb 24, 2024
Made some improvements to the render deploy command while debugging an issue with realtime on their platform (see https://community.redwoodjs.com/t/redwood-realtime-on-render-com/5796 for the original issue). Note that while this PR fixes the original issue (subscriptions is not an allowed operation), realtime itself still doesn't work as expected. Currently trying to isolate render if it is specific to them. But these are valid fixes in the meantime: - should improve the memory usage problem by 1) requiring cli-data-migrate to be a devDependency prior to deploy and 2) not importing from the cli's lib which brings in the whole world 3) not using execa's shell option which isn't recommended - render will now use the server file if it's there
jtoar
added a commit
that referenced
this pull request
Feb 26, 2024
Made some improvements to the render deploy command while debugging an issue with realtime on their platform (see https://community.redwoodjs.com/t/redwood-realtime-on-render-com/5796 for the original issue). Note that while this PR fixes the original issue (subscriptions is not an allowed operation), realtime itself still doesn't work as expected. Currently trying to isolate render if it is specific to them. But these are valid fixes in the meantime: - should improve the memory usage problem by 1) requiring cli-data-migrate to be a devDependency prior to deploy and 2) not importing from the cli's lib which brings in the whole world 3) not using execa's shell option which isn't recommended - render will now use the server file if it's there
jtoar
added a commit
that referenced
this pull request
Feb 27, 2024
Similar to #10055, updates the setup command for Coherence to detect the server file. This deployed successfully in this commit on deploy target CI here: redwoodjs/deploy-target-ci@4e5883e.
jtoar
added a commit
that referenced
this pull request
Feb 27, 2024
Similar to #10055, updates the setup command for Coherence to detect the server file. This deployed successfully in this commit on deploy target CI here: redwoodjs/deploy-target-ci@4e5883e.
jtoar
added a commit
that referenced
this pull request
Feb 28, 2024
Similar to #10055, updates the setup command for Coherence to detect the server file. This deployed successfully in this commit on deploy target CI here: redwoodjs/deploy-target-ci@4e5883e.
jtoar
added a commit
that referenced
this pull request
Feb 28, 2024
Similar to #10055, updates the setup command for Coherence to detect the server file. This deployed successfully in this commit on deploy target CI here: redwoodjs/deploy-target-ci@4e5883e.
jtoar
added a commit
that referenced
this pull request
Feb 28, 2024
Similar to #10055, updates the setup command for Coherence to detect the server file. This deployed successfully in this commit on deploy target CI here: redwoodjs/deploy-target-ci@4e5883e.
jtoar
added a commit
that referenced
this pull request
Feb 28, 2024
Similar to #10055, updates the setup command for Coherence to detect the server file. This deployed successfully in this commit on deploy target CI here: redwoodjs/deploy-target-ci@4e5883e.
jtoar
added a commit
that referenced
this pull request
Feb 29, 2024
At first this PR was similar to #10055 but for Flightcontrol. But in the process of testing that and updating deploy target CI, I noticed a few other things were off: - coherence logic for the server file was flipped. fixed here - in general, a few duplicate ways of checking for the server file. tried to dedupe them the best i could without making massive changes - stylistic change to render - the flightcontrol setup wasn't handing corepack Test as much as I could locally. Going to get this one into next so I can test in deploy target CI.
jtoar
added a commit
that referenced
this pull request
Feb 29, 2024
At first this PR was similar to #10055 but for Flightcontrol. But in the process of testing that and updating deploy target CI, I noticed a few other things were off: - coherence logic for the server file was flipped. fixed here - in general, a few duplicate ways of checking for the server file. tried to dedupe them the best i could without making massive changes - stylistic change to render - the flightcontrol setup wasn't handing corepack Test as much as I could locally. Going to get this one into next so I can test in deploy target CI.
jtoar
added a commit
that referenced
this pull request
Feb 29, 2024
At first this PR was similar to #10055 but for Flightcontrol. But in the process of testing that and updating deploy target CI, I noticed a few other things were off: - coherence logic for the server file was flipped. fixed here - in general, a few duplicate ways of checking for the server file. tried to dedupe them the best i could without making massive changes - stylistic change to render - the flightcontrol setup wasn't handing corepack Test as much as I could locally. Going to get this one into next so I can test in deploy target CI.
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.
Made some improvements to the render deploy command while debugging an issue with realtime on their platform (see https://community.redwoodjs.com/t/redwood-realtime-on-render-com/5796 for the original issue). Note that while this PR fixes the original issue (subscriptions is not an allowed operation), realtime itself still doesn't work as expected. Currently trying to isolate render if it is specific to them. But these are valid fixes in the meantime: