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

Adding .python-compile-env for to support exporting env vars for pip install #750

Closed
wants to merge 2 commits into from

Conversation

tomasbielskis
Copy link

I added the ability to define config variables required for pip install to work for airflow 1.10. Airflow now requires the environment variable SLUGIFY_USES_TEXT_UNIDECODE to be defined and exported for app build in order for airflow to install.

By creating a file .python-compile-env in the app source root, it will be sourced before pip install runs. The file simply exports environment variables:

export SLUGIFY_USES_TEXT_UNIDECODE=yes

@CaseyFaist
Copy link
Contributor

Thanks for the PR!

Now, I'm not sure adding library specific logic to the buildpack is the best route to this functionality. I think the underlying problem (environment variables created before pip install) has merit, but I'd sooner adopt something that can be adapted to solve similar problems for other use cases too.

Towards that goal, can you describe how this solves the Airflow problem in more detail? In particular:

@tomasbielskis
Copy link
Author

tomasbielskis commented Sep 17, 2018

Hi @CaseyFaist!

We tried using the config vars first, but they don't seem to be exposed to the buildpack in the environment itself. In fact, the documentation suggests exactly that:

The application config vars are passed to the buildpack as an argument (versus set in the environment) so that the buildpack can optionally export none, all or parts of the app config vars available in ENV_DIR when the compile script is run.

...So even if we wanted to use config vars, we'd still have to modify the buildpack to extract what we want, which would be very proprietary.

We can't set anything in the os library because that happens after install and the environment variable need to be present in order to install.

We tried to solve this in the most proprietary software-agnostic way, if there's another way to accomplish this we're totally open to something better!

@edmorley
Copy link
Member

For previous art on this topic, see:
#417
#467
#639

@ghost
Copy link

ghost commented Sep 17, 2018

@edmorley I read the changes in those to see what I could glean. It looks like in #639 you did more or less what we changed (if file exists, read it into environment variable). The variable we're defining is proprietary (SLUGIFY_USES_TEXT_UNIDECODE) so we're being a little more willy-nilly by simply sourcing the file. This isn't ideal because the file can contain literally anything, but we haven't yet identified a better solution.

The bottom line is we can't use airflow 1.10 without this environment variable. There's no support in the buildpack to allow for arbitrary environment variables to be made available to pip install. I feel like this change is not palatable. Could you give us some specific feedback as to what we can do to make this acceptable?

@edmorley
Copy link
Member

I didn't write those PRs, nor do I have an opinion either way on this topic (and am not a Heroku employee); those links just seemed relevent to the discussion in case either of you hadn't seen them already :-)

@ghost
Copy link

ghost commented Sep 17, 2018

I got you, thanks!

@CaseyFaist
Copy link
Contributor

CaseyFaist commented Sep 17, 2018

Thanks for expanding @tomasbielskis and for more context @edmorley! I hadn't seen those PRs, I'm fairly new at Heroku myself :) I <3 airflow though, so I'm glad this issue was surfaced.

@lmcgrath not unpalatable - I just need to test for side effects and write some tests

Build is failing b/c Travis tests need privileged creds to run. I rolled the commit over to new PR here: #751 please continue discussion over there.

@CaseyFaist
Copy link
Contributor

Closing in favor of #760 👍

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

3 participants