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

Only copy from app import path to app path when building #467

Merged

Conversation

mlandauer
Copy link
Contributor

@mlandauer mlandauer commented Jul 17, 2019

This PR fixes #402 (slug generate gzips from /tmp/app, when it should gzip from /app) and #321 (Cannot execute any Procfile commands) and fixed the issue mentioned in the blog post "Running Herokuish apps in Docker".

Currently every time herokuish is run the app import path (e.g. /tmp/app) is copied over to the app path (e.g. /app) if the import path exists and has files in it.

This PR changes this so that the copy from the import path to the app path is only done on running herokuish buildpack build or herokuish buildpack test as suggested by @matthewmueller in #402.

This means that you can now run buildpack build immediately followed by procfile start within the same container and things will work.

I can see at least two different ways that people were working around this issue before:

  1. Mount the application volume in the app directory (/app) rather than the import app directory (/tmp/app) as seen in Dokku. This has the disadvantage that build artifacts get written into the same directory as the source code (of course you might want this, you might not).
  2. By using the recommended flow as outlined by @michaelshobbs, you create a docker image from the result of running buildpack build and then running a container from that image which doesn't have a volume mounted at /tmp/app and so the contents of /app is not overwritten.

This PR should not break those workarounds.

@josegonzalez
Copy link
Member

Assigning to @michaelshobbs for review :)

@michaelshobbs
Copy link
Member

@josegonzalez this should be tested with dokku before upgrading the default version

@mlandauer
Copy link
Contributor Author

@josegonzalez is there anything I can do to help with the dokku testing?

@matthewmueller
Copy link

@mlandauer thanks for PRing this – I'd love to see this pushed through!

@josegonzalez
Copy link
Member

Testing in dokku would require building the herokuish image and pushing a few apps with it.

@josegonzalez
Copy link
Member

I am pretty sure this will work based on a single test of a python app, but will try and get this more stress-tested soon.

@sriprasanna
Copy link

Any chance of this to be released in the near future? I encountered the same problem and it would be great to have this merged. This change will certainly come in handy when I want to chain multiple herokuish commands.

@josegonzalez josegonzalez force-pushed the only_copy_to_app_path_on_build branch from febc15e to 7a1c10f Compare March 28, 2024 21:06
@josegonzalez
Copy link
Member

This worked locally with Dokku like so:

BUILDX=false make build/docker/22 STACK_VERSION=22 && docker image tag herokuish:latest-22 gliderlabs/herokuish:latest-22 && tput bel

@josegonzalez josegonzalez merged commit f75971f into gliderlabs:master Mar 28, 2024
35 checks passed
josegonzalez added a commit that referenced this pull request Mar 28, 2024
- #1093 @dependabot: chore(deps): bump actions/download-artifact from 3 to 4
- #1173 @josegonzalez: Use find to identify only files not already owned by user
- #1174 @josegonzalez: Add linting to CI
- #1175 @josegonzalez: Ensure all file permissions are set to specified unprivileged user
- #467 @mlandauer: Only copy from app import path to app path when building
@josegonzalez
Copy link
Member

Apologies for getting to this so late, just didn't have the bandwidth/mental energy to work on it.

This was referenced Mar 28, 2024
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.

slug generate gzips from /tmp/app, when it should gzip from /app
6 participants