search

gjaldon / heroku-buildpack-phoenix-static

A Heroku buildpack for building Phoenix's static assets
MIT License
229 stars 224 forks source link

Use a test config file for Heroku CI runs #118

Closed jorbs closed 3 years ago

jorbs commented 3 years ago

This PR sources phoenix_static_buildpack.test.config if it is a Heroku CI build by overriding the config values in phoenix_static_buildpack.config.

This is useful when, for example, we want to keep the cache folder for CI builds, but not for production builds.

jorbs commented 3 years ago

@jesseshieh I kindly ask for your review :)

jesseshieh commented 3 years ago

Hey @jorbs, thanks for the pull request. I'm not sure I completely follow your use case. Can you elaborate a little bit on why this is needed?

jorbs commented 3 years ago

Hey @jorbs, thanks for the pull request. I'm not sure I completely follow your use case. Can you elaborate a little bit on why this is needed?

@jesseshieh Sure! First off, now I think this PR is not needed.

I will explain the motivation for it using my application scenario as example.

Some days ago, we were having issues with production builds because newly JS libraries versions added to package.json weren't being installed. So we decided to fix this issue by setting clean_cache=true in our phoenix_static_buildpack.config.

But other problem came up from that fix: for every Heroku CI run, the buildpack wasn't using the cached folder due to the setting above. This increased the CI execution time. So I decided to create this PR and common.sh script to read a separate test file (phoenix_static_buildpack.test.config) with clean_cache=false that would be read only by the CI environment.

Does it make sense?

Now, thinking a bit more, I think the PR is not needed. Because we can use clean_cache=false in a single file and if we think we need to rebuild the cache, I can simply clear the cache manually like https://help.heroku.com/18PI5RSY/how-do-i-clear-the-build-cache.

jesseshieh commented 3 years ago

Oh I see, thanks for explaining! Can this issue be closed then?