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

Var in release #88

Merged
merged 5 commits into from
Mar 14, 2019
Merged

Var in release #88

merged 5 commits into from
Mar 14, 2019

Conversation

rccursach
Copy link

passed phoenix_ex var from lib/common.sh#load_config() to bin/release through a small file generated in $BUILD_DIR.

Calling load_config() from inside release caused error, because of the messages echoed from load_config() were considered part of the YAML output generated by the release script.

also changed the logic of load_config(), bc phoenix 1.3.x apps have a package.json on ./assets/, that wasn't detected. and moved the sourcing of phoenix_static_buildpack.config at the end to override what was already set from the defaults if the file is present.

Minor change since it looks like $BUILD_DIR/PHOENIX_EX_VAR is supposed to exist so we can remove the conditional to check if it exists
@gjaldon
Copy link
Owner

gjaldon commented Mar 13, 2019

This is awesome @rccursach! Thanks a lot :) Let me know if you've tested with a deploy and if it's good to merge. Hope you don't mind the minor change I in bin/release.

Should add automated tests and CI for this buildpack so we no longer need to manually test in the future

@rccursach
Copy link
Author

Yeah, makes sense removing that IF, the var is already there always.

Good to merge :). Tested with 1.3.x on heroku and dokku and works fine .

(Also looked in other Buildpacks for how testing is done. it's a lot of work, but the buildpacks from heroku have pretty consistent code base, independently of what framework/lang are targeted for. 👓)

@gjaldon gjaldon merged commit a04f40b into gjaldon:var-in-release Mar 14, 2019
@gjaldon
Copy link
Owner

gjaldon commented Mar 14, 2019

Awesome! Thanks a lot for this contribution 💜

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.

None yet

2 participants