Closed som-matrix closed 6 months ago
Folks please check this ^
+1
Hey @som-matrix do you know if this fixes the problem for Elixir Applications getting mix exec path
error? Is there a way to use your branch on my yml script? Thanks in advance.
As far as I'm aware, this change is highly backward incompatible and will break all existing uses.
Hey @som-matrix do you know if this fixes the problem for Elixir Applications getting
mix exec path
error? Is there a way to use your branch on my yml script? Thanks in advance.
Hey @ricksonoliveira i haven't came across this mix exec path
error , because we are using this action in a Ruby on Rails project. But i see here https://github.com/superfly/fly-pr-review-apps/issues/22 they have mentioned to use flyctl apps create
instead flyctl launch
. Did you tried this ?
If it works i will also probably try using flyctl apps create
in our project and check if it is working smoothly or not.
Thanks for the shoutout man !
As far as I'm aware, this change is highly backward incompatible and will break all existing uses.
Hey @felixxm thanks for the review , it would be nice if you can point me to the code that will break the existing cases ? How is it backward incompatible 🤔 ? Can you please explain that ?
Hey @som-matrix, they did try this approach here #46 but it seems it didn't work, they didn't even merge it. Anyways if I do try flyctl apps create
I wouldn't be able to update my preview app if I made a new commit in the same opened PR would I? Because it would create a whole new app on fly.io and maybe even with the same pr number so might even fail? I had this issue trying my own script but using flyctl apps create
, I was getting name already taken
when I tried updating a PR.
Hey @som-matrix, they did try this approach here #46 but it seems it didn't work, they didn't even merge it. Anyways if I do try
flyctl apps create
I wouldn't be able to update my preview app if I made a new commit in the same opened PR would I? Because it would create a whole new app on fly.io and maybe even with the same pr number so might even fail? I had this issue trying my own script but usingflyctl apps create
, I was gettingname already taken
when I tried updating a PR.
@ricksonoliveira flyctl apps create
will only run for the first commit, for subsequent commits later on to the PR will not execute that command.
When you push new commits it will execute this flyctl deploy $detach --app "$app" --region "$region" --strategy immediate --remote-only
and in yml file as mentioned we have to add the step to update the image , which will make sure preview app is updated with the changes made in the commit you pushed.
Thanks for the explanation on this @som-matrix. Understood. The only problem for me now is the fact that Phoenix applications are getting mix exec path
error.
Hey @som-matrix I was able to fork this repo and create my own, adding your changes to test them, I made some mods for instance to create a pg database app if it still doesnt exist for that pr, you can take a look if you want on my repo to check out more about it: https://github.com/ricksonoliveira/fly-pr-review-apps
It's finally deploying! 🚀
But the problem is that the app is staying idle after deployment, like in the screenshot I sent below, do you know what could be the issue here, why isn't it making the app go live?
@ricksonoliveira cool, good job 👍🏼
i see , can you share a screenshot of this action when running in github ?
It is showing pending
state which means the app is still not deployed, it only executed the create
step.
Hey @som-matrix I finally got around it! The problem was that using the secrets as you suggested in the README doesn't work for Elixir Phoenix apps because we need to set up a secret before the flyctl deploy
command, and this is automatically done in the fly launch so that's why it wasn't working for me. So what I did was tweak the entrypoint.sh
script to add arguments to add these secrets, and they're later embedded in the flyctl deploy
comand.
Anyways if you have suggestions feel free to let me know or maybe even collaborate. Thanks!
imo if we want this pr to get merged, it should be cut in different parts. I mean one PR per feature. like:
As felixxm stated above, this PR is not mergeable as is. my advice, close it and suggest features one by one (PR by PR) personally I do not need anything from this PR and was mislead by reading some issue where someone was saying to try out this PR. but I find the detach feature smart (for non paying private repos) and think the documentation definitely needs to be clearer: like for the need for an org token
imo if we want this pr to get merged, it should be cut in different parts. I mean one PR per feature. like:
* one for the detach feature * one for the dockerfile append to the deploy command. * one for the documentation that emphasis that we need an org token and not just a machine token * one to change the name of the machine created with fly launch * etc...
As felixxm stated above, this PR is not mergeable as is. my advice, close it and suggest features one by one (PR by PR) personally I do not need anything from this PR and was mislead by reading some issue where someone was saying to try out this PR. but I find the detach feature smart (for non paying private repos) and think the documentation definitely needs to be clearer: like for the need for an org token
Yeah pushing one feature at a time sounds good to me. But you have said etc
what other things i need to push ?
you mean considering litefs
and multiple toml file ?
Closing this PR and will open small PR's feature by feature as mentioned here -> https://github.com/superfly/fly-pr-review-apps/pull/48#issuecomment-2028079524
Summary
This PR introduces several enhancements and modifications to our Fly.io deployment setup and GitHub actions workflow. The primary goal is to streamline the process, improve clarity in naming conventions, and incorporate additional functionalities for more flexible deployments. Below is a detailed overview of the changes made.
Dockerfile Changes
Added Bash: The Alpine base image now includes the bash package alongside curl and jq. This addition supports scripts that require Bash for more complex operations than what sh can handle.
README.md Updates
{repo_name}-pr-{number}
.action.yml
Enhancements
Streamlined Inputs: Simplified the action's inputs by removing redundant ones and introducing the dockerfile input for specifying Dockerfile paths directly. This change aligns with the README updates and provides clearer options for action users.
entrypoint.sh
Bash Shebang
Flyctl Version Check
Simplified App Naming Convention
Dedicated Variable for Postgres Applications
Direct Handling of Dockerfile Paths
Implemented support for specifying custom Dockerfile paths, increasing flexibility in Docker image construction.
Optional Detached Deployment
Added an option for detached deployment execution, allowing CI/CD processes to proceed without waiting for deployment completion.
Removed Postgres Cluster Creation
Moved Secret Setting to YAML
Fly Image Update Step
Simplified Resource Configuration
App Destruction on PR Closure
These changes aim to streamline the deployment process on Fly.io, enhance reliability, simplify configuration management, and improve the overall efficiency of handling PR-based review applications.
These changes collectively enhance the script's usability, flexibility, and clarity, aligning with updates to the GitHub Action for more efficient Fly.io deployments.
Motivation
These changes aim to make our deployment process more robust, flexible, and user-friendly. By incorporating feedback and identifying areas for improvement, we've updated the deployment strategy to better meet our development and review needs.
Testing
Please ensure to test the deployment process in a staging environment before merging. Verify that the new naming conventions are applied correctly and that custom Dockerfile paths are respected during the build process.
Feel free to adjust the wording as necessary to fit the context of your project or to add any additional details that are relevant to your changes. 🙂