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

Use fly launch/dockerfile-node instead of .dockerignore, Dockerfile, fly.toml & start.sh #252

Open
MichaelDeBoey opened this issue Aug 23, 2023 · 9 comments

Comments

@MichaelDeBoey
Copy link
Member

As @rubys mentioned in #232 (comment) & #232 (comment), we should be able to get rid of .dockerignore, Dockerfile, fly.toml & start.sh in favor of using fly launch/dockerfile-node:

Any chance remix could consider switching to dockerfile node?

I believe we can support everything. If we discover something that needs to be added that can be done quickly, so I'm not worried about that.

This would mean that we also need to update our README to not include the now obsolete steps

The doc changes should be pretty straightforward. Much of the current README can be replaced by fly launch.

We will also need to update our deploy workflow so that it runs fly launch/dockerfile-node each time in the deploy step

We can probably use https://github.com/fly-apps/dockerfile-node/actions/runs/5933428308/job/16088842745 as an inspiration for that

If looks like your existing test deploys the app on fly.io. It doesn't create the application, so it presumes that the app is already present?

So the next step is presumably to figure out what test is needed, subtract out what is already covered, and then write a test for what remains. Whether that test or tests resides in a remix repository or a fly apps repository doesn't matter to me.

The vitest step doesn't need Fly at all, but in the deploy step we indeed need Fly.

There are indeed 2 existing applications named indie-stack-template & indie-stack-template-staging which are deploying latest code changes (indie-stack-template deploys main branch, indie-stack-template-staging deploys dev branch, which actually doesn't exist in this project but is there for convenience when using this stack)
If I'm correct @kentcdodds set these up when creating these, so I'm not sure under which account they're available if more info is needed about them

The idea of the deploy workflow is indeed that the applications are already set up on Fly and that you're just pushing new code, not that you create a new app every time you update your existing codebase


Reference: https://github.com/fly-apps/dockerfile-node

@kentcdodds
Copy link
Member

The apps are under the Remix fly account.

I have no issues with any changes to this stack so long as it works and is as simple or simpler

@mikeybinns
Copy link

mikeybinns commented Aug 23, 2023

One modification I always make to my build is to delete certain routes on deploy in the Dockerfile just before build, without deleting them locally / in the git repo. On route convention v1, this was a "_test" folder, and on route conventions v2, it's any files that start with _test.. This provides me a way to create a helpful for testing route, but ensure it doesn't leak onto production builds.

Is there a way to do this kind of code manipulation with dockerfile-node? Obviously if not, I don't expect for this to block any implementation, I'm just not 100% clear how dockerfile-node will work with regards to edits like this.

In case this isn't clear, this is the "Build the app" code from the Dockerfile for routes v2:
image

@MichaelDeBoey
Copy link
Member Author

I have no issues with any changes to this stack so long as it works and is as simple or simpler

@kentcdodds The purpose of @rubys' suggestion is to have less files in the stack/project and let fly launch/dockerfile-node do the heavy stuff that's needed for deploying to Fly

@rubys
Copy link

rubys commented Aug 23, 2023

One modification I always make to my build is to delete certain routes on deploy in the Dockerfile just before build

I would suggest putting this in .dockerignore?

In any case, the answer is yes: dockerfile-node is just a bunch of ejs templates, and it would be no problem to add something like:

<% if (remix) { -%>
# remove test route files from non-local builds
RUN if [ ... ] then ... fi
<% } -%>

@MichaelDeBoey
Copy link
Member Author

@rubys I would suggest to support this in a generic way, like the extra dependencies that can be installed for certain stages of Dockerfile

@mikeybinns
Copy link

I would suggest putting this in .dockerignore?

omg 🤦 I can't believe I didn't think of this... This definitely would be a much easier way to do this for my use case.

I would suggest to support this in a generic way

I agree with this, I was mainly giving my use-case only an example, and I'm sure there's other things people may want to do just before or after a build.

Maybe you could provide a way to hook into the process at certain points, e.g. before_build, after_build, after_deploy where uses can execute code. I don't know anything about ejs, but seeing as it's just JS, maybe you can make it possible to just provide a function which accepts parameters like buildFiles, context, buildArgs?

@rubys
Copy link

rubys commented Aug 24, 2023

Maybe you could provide a way to hook into the process at certain points, e.g. before_build, after_build, after_deploy where uses can execute code. I don't know anything about ejs, but seeing as it's just JS, maybe you can make it possible to just provide a function which accepts parameters like buildFiles, context, buildArgs?

Good news. Take a look at the current set of options: https://github.com/fly-apps/dockerfile-node#options

  • --instructions lets you specify a file to inserted in one of three places in the Dockerfile
  • --arg lets you specify build args

Can you describe what you mean by buildFiles and context? If those are generic, they can be added as flags and can benefit all node applications, not just remix ones.

@mikeybinns
Copy link

Ah amazing :) yeah I think that was exactly what I was envisioning, sorry next time I'll do a deeper dive into the documentation :)

Don't worry about those parameters as they are likely irrelevant with your current way of doing things :)

@mikeybinns
Copy link

In case this doesn't go ahead for whatever reason, there's some other updates required for the fly.toml.

As far as is documented, allowed_public_ports and auto_rollback are not valid parameters. I discovered this because "auto_rollback" didn't do anything when I accidentally deployed a bad build.

To replace auto_rollback, we should set a deploy strategy as shown here:

[deploy]
  strategy = "bluegreen"

https://fly.io/docs/reference/configuration/#picking-a-deployment-strategy

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

No branches or pull requests

4 participants