Skip to content
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

Override serverUrl in wasp deploy fly deploy command #2234

Merged
merged 6 commits into from
Sep 2, 2024

Conversation

Case-E
Copy link
Contributor

@Case-E Case-E commented Aug 10, 2024

Description

Fixes #2233

Select what type of change this PR introduces:

  1. Just code/docs improvement (no functional change).
  2. Bug fix (non-breaking change which fixes an issue).
  3. New feature (non-breaking change which adds functionality).
  4. Breaking change (fix or feature that would cause existing functionality to not work as expected).

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:

  1. I updated ChangeLog.md with description of the change this PR introduces.
  2. I bumped waspc version in waspc.cabal to reflect changes I introduced, with regards to the version of the latest wasp release, if the bump was needed.

Update example apps if needed

If you did code changes and added a new feature or modified an existing feature, make sure you satisfy the following:

  1. I updated waspc/examples/todoApp as needed (updated modified feature or added new feature) and manually checked it works correctly.
  2. I updated waspc/headless-test/examples/todoApp and its e2e tests as needed (updated modified feature and its tests or added new feature and new tests for it).

WORKAROUND for anyone who needs this before it is merged

  1. Make sure that you have flyctl installed and logged in. You can run flyctl auth whoami
  2. Run these commands from the root of your project
wasp build
  1. Navigate to the build folder
cd .wasp/build/web-app
  1. Copy the client config file into the build dir
cp ../../../fly-client.toml ./fly.toml
  1. Install dependencies and build the client
npm install && REACT_APP_API_URL=https://sub.domain.com npm run build
  1. Create the Dockerfile and add the following contents
touch Dockerfile
FROM pierrezemb/gostatic
CMD [ "-fallback", "index.html" ]
COPY ./build/ /srv/http/
  1. Create the docker ignore file
touch .dockerignore
  1. Deploy with this command. (optional: you can pass --local-only in place of --remote-only to build locally)
flyctl deploy --remote-only

@Case-E
Copy link
Contributor Author

Case-E commented Aug 12, 2024

The tests were failing even without the changes, so not introduced by the changes in this PR.

@Case-E
Copy link
Contributor Author

Case-E commented Aug 12, 2024

It looks like I am unable to test this locally. Not sure why the deploy command seems to be throwing this error:

Screen Shot 2024-08-12 at 6 12 20 PM

wasp-cli: npm: readCreateProcessWithExitCode: chdir: invalid argument (Bad file descriptor)

Other wasp-cli commands seem to be working. Tested build and start

@Case-E Case-E closed this Aug 12, 2024
@Case-E Case-E force-pushed the 2233-fix-serverUrl-wasp-deploy-fly branch from bf982bd to 1f75746 Compare August 12, 2024 14:27
@Case-E Case-E reopened this Aug 12, 2024
@Case-E
Copy link
Contributor Author

Case-E commented Aug 12, 2024

Everything works as expected ]. will bump the version and update the changelog.

@infomiho
Copy link
Contributor

infomiho commented Aug 19, 2024

Hey @Case-E, thank you for the contribution.

We'll need to do a few things before we can merge this:

  • You'll need to sync your fork to make sure it includes the latest code
  • You'll need to merge main into your branch
  • I'll need to review the code changes and test it out locally
  • You'll need to make sure the CI is passing (probably syncing the fork will do the trick)

Copy link
Contributor

@infomiho infomiho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested it locally, works as expected.

Left some minor comments.

waspc/packages/deploy/src/providers/fly/deploy/deploy.ts Outdated Show resolved Hide resolved
waspc/packages/deploy/src/providers/fly/deploy/deploy.ts Outdated Show resolved Hide resolved
waspc/packages/deploy/src/providers/fly/helpers/helpers.ts Outdated Show resolved Hide resolved
@Case-E
Copy link
Contributor Author

Case-E commented Aug 19, 2024

Thanks @infomiho I didn't realise there were more changes pushed upstream from when I branched out.

I've made the changes. As this needs to go into the current release, do I need to update the version (patch) or would y'all be batching multiple fixes into a single patch? Also should I add to the changelog?

@Case-E
Copy link
Contributor Author

Case-E commented Aug 19, 2024

Looks like the CI failed to cancel out existing running jobs, y'all may want to check why or add it to your tech debt.
https://github.com/wasp-lang/wasp/actions/runs/10456113609/job/28952663383?pr=2234

Screenshot 2024-08-19 at 9 01 14 PM

@Case-E
Copy link
Contributor Author

Case-E commented Aug 19, 2024

Added a workaround to the description in case anyone needs it before the changes are merged.

@infomiho
Copy link
Contributor

infomiho commented Sep 2, 2024

@Case-E sorry for taking so long to get back to you, thank you for your hard work on this.

I realised now that we can remove the use of stripTrailingSlash since the client does that before it uses the env var: https://github.com/wasp-lang/wasp/blob/main/waspc/data/Generator/templates/sdk/wasp/client/config.ts#L4

So, remove that helper and the use of it and we'll merge it! I'll update the Changelog with this change later.

@Case-E
Copy link
Contributor Author

Case-E commented Sep 2, 2024

@infomiho No sorry needed at all! I didn't venture into the client code, so that all makes sense. I've removed the helper now.

@Case-E
Copy link
Contributor Author

Case-E commented Sep 2, 2024

When you get a chance, could you also do an initial review of the deploy action — wasp-lang/deploy-action#2? I haven't tested that yet though, will do it after this change is merged and released.

@infomiho infomiho merged commit 021cc31 into wasp-lang:main Sep 2, 2024
6 checks passed
@Case-E Case-E deleted the 2233-fix-serverUrl-wasp-deploy-fly branch September 3, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Server URL gets reset on the client in new deployments on Fly
2 participants