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

Populate environment variables as needed, fixes #15, fixes #16 #17

Merged
merged 11 commits into from
Jul 22, 2022

Conversation

rfay
Copy link
Member

@rfay rfay commented Jun 30, 2022

First work on trying to populate environment variables, including PLATFORM_ROUTES

Unfortunately, I'm probably stuck. Will need to do a collaborative debugging session to understand this I think.

@rfay rfay marked this pull request as draft July 1, 2022 00:32
@lolautruche
Copy link
Contributor

Sure, let me know how I can help!

@rfay
Copy link
Member Author

rfay commented Jul 1, 2022

@lolautruche this currently does a lot of things against your bigfoot-symfony, example: https://gist.github.com/rfay/35394655f57b5599fa1c1923887859f4

There are lots of things to be learned in trying out these hooks (now including build hooks):

  1. ddev doesn't have a concept of "build". It does have Dockerfile, but that is not done in the context of code, so most of the things you're doing in build can't be done here. We may have to rethink ddev's hooks to somehow introduce things that won't have to be done on every start.
  2. The Platform build hooks expect a database to already be there. ddev's post-start hooks don't have any assumptions like that.
  3. You'll see that your hooks fail in https://gist.github.com/rfay/35394655f57b5599fa1c1923887859f4#file-bigfoot-startup-sh-L143 trying to set up nvm, with an assumption of where it will be. But DDEV already has an nvm feature, and not necessarily compatible.
  4. There are some other assumptions, like .global/bin being in the $PATH and tmp/prod not having already been populated.

Overall, these don't make me optimistic of being able to cover all (or many perhaps) of the cases of importing hooks from Platform. There are just too many assumptions built into scripts that are written for Platform.

Anyway, let's talk through this and the failures and try it out.

You can experiment with this using ddev get https://github.com/rfay/ddev-platformsh/tarball/20220630_populate_env

(You'll need to remove the opcache setting in your custom php to get ddev to come up)

@lolautruche
Copy link
Contributor

Thanks @rfay !

  1. It's OK that ddev doesn't have a concept of build. We might indeed consider think about ddev's hooks, but AFAICT Platform hooks are run every time a deploy is triggered, so it feels OK to have them running on every start.
  2. The build hook doesn't expect a database to already be there, but the deploy and post-deploy hooks do.
  3. This is coming from the Symfony script, we may figure this out with @fabpot .
  4. It's true that .global/bin is expected to be in $PATH. Sorry about that, I forgot to tell you. About /tmp/prod, I'm not sure to get what you're talking about.

I'm still pretty sure we can cover most (if not all) of the cases with hooks 🙂

@rfay
Copy link
Member Author

rfay commented Jul 5, 2022

Oh, I note

  • the symfony script is also doing a pkill -f php-fpm, which brings down the container, as this is unexpected. There are a lot better ways to do a reload (pkill -USR2 for example)
  • The cloud-configurator seems to have a bug, bash: line 26: warning: here-document at line 18 delimited by end-of-file (wanted EOS')`

If you'll link me to these I'll do a PR. @fapot would love to have your input on those issues.

@lolautruche
Copy link
Contributor

It seems that we're missing an environment file which is expected to be sourced (/app/.global/environment).
It contains the following:

export MIX_HOME="$PLATFORM_APP_DIR/.global/mix"
export COMPOSER_HOME="$PLATFORM_APP_DIR/.global"
export NPM_CONFIG_PREFIX="$PLATFORM_APP_DIR/.global"
export PATH="$PLATFORM_APP_DIR/.global/bin:$PLATFORM_APP_DIR/.global/composer/composer/vendor/bin:$PLATFORM_APP_DIR/.global/vendor/bin:$PATH"
export GEM_PATH="$PLATFORM_APP_DIR/.global:$PLATFORM_APP_DIR:/usr/share/rubygems-integration/all"
export PYTHONUSERBASE="$PLATFORM_APP_DIR/.global"

@rfay
Copy link
Member Author

rfay commented Jul 5, 2022

That will be huge, thanks. Who expected that to be sourced? Platform? It's easy to do, looks like it will solve all kinds of problems, especially the PATH problem, which was kind of ugly.

@lolautruche
Copy link
Contributor

Also, containers may have /app/.environment files, which are supposed to be sourced whenever present.
It's supposed to contain shell, like a .bashrc

@rfay
Copy link
Member Author

rfay commented Jul 5, 2022

The EOS error is from https://get.symfony.com/cloud/configurator - there is no EOS. @fabpot I'll do a PR if I know where that is on github. It's a bug that should be fixed.

@rfay
Copy link
Member Author

rfay commented Jul 5, 2022

I sent an email to Fabien about the cloud configurator, copied you @lolautruche

@rfay
Copy link
Member Author

rfay commented Jul 9, 2022

@fabpot kindly fixed the two outstanding items with cloud configurator - not killing off php-fpm and instead reloading and the missing EOS.

There are a couple of other items now that I emailed him about,

1. Could you please change line 46 of .global/bin/symfony-build

export NVM_DIR=${APP_DIR}/.nvm

to be

export NVM_DIR=${NVM_DIR:-${APP_DIR}/.nvm}

so it doesn't overwrite NVM_DIR if it's already set?

DDEV already has NVM support and nvm is set up.


2. It seems the cloud configurator installs the platform cli unconditionally - shouldn't it check to see if it's already installed first? (line 16) Maybe instead of 

(curl -fsS https://platform.sh/cli/installer | php -n -d"$(php -n -r 'extension_loaded("json")||print("extension");')"=json.so -d extension=phar.so -d extension=mbstring.so -d extension=posix.so -d extension=curl.so) || true

if [ ! command -v platform >/dev/null ] ; then 
  (curl -fsS https://platform.sh/cli/installer | php -n -d"$(php -n -r 'extension_loaded("json")||print("extension");')"=json.so -d extension=phar.so -d extension=mbstring.so -d extension=posix.so -d extension=curl.so) || true
fi

Again, if there's a way I can fix this with a PR I'll happily do it.

@rfay
Copy link
Member Author

rfay commented Jul 9, 2022

@lolautruche the default drupal template hooks have drush -y config-import without specifying the source of the config, so it always fails with " This import is empty and if applied would delete all of your configuration, so has been rejected."

This command results in the same output if executed in the Platform.sh site.

Shouldn't this be specifying a config for import? Or something?

@rfay
Copy link
Member Author

rfay commented Jul 9, 2022

Some of the handling of environment is starting to mature, and I've moved it to a homedir setup, but there are still things that don't work across all the projects I'm working with (and my platform token doesn't even get your project any more for no reason I understand)

Can we spend an hour or two debugging through environment variables in different contexts?

@lolautruche
Copy link
Contributor

Shouldn't this be specifying a config for import? Or something?

If this also happens on Platform, it means that we probably have a bug in our template (ping @chadwcarlson @gilzow @thomasdiluccio). You can find the template source here: https://github.com/platformsh/template-builder/tree/master/templates/drupal9/files

Can we spend an hour or two debugging through environment variables in different contexts?

Sure, let's do this today during our call, together with @gilzow 🙂

@rfay
Copy link
Member Author

rfay commented Jul 11, 2022

Thanks - @fabpot fixed the things I emailed about as well. See you today!

@rfay
Copy link
Member Author

rfay commented Jul 11, 2022

  • Fabien: Wrong fix on command -v
  • Browserslist: caniuse-lite is outdated. Please run:
  • PLATFORM_APPLICATION_NAME must be real, and shouldn’t be just “drupal” all the time.
  • Can PLATFORM_APPLICATION_NAME go in bash.d?
  • Need mounts in pull - pull all of them (ddev pull platform should pull all the mounts #19)
  • PLATFORM_PROJECT_ENTROPY may not work with logins or other things that use salt, unless …
  • Best way to get all env is PLATFORM_APPLICATION and decode it. BUT /run/config.json has everything including entropy. Concern: Are there security implications of this file?
  • I see PLATFORM_DIR in config.json, not PLATFORM_APP_DIR. However, per @damz in https://platformsh.slack.com/archives/CEDK8KCSC/p1657555263719799 $PLATFORM_APP_DIR is the only one to be considered valid.
  • PLATFORM_ENVIRONMENT is normally the branch name
  • PLATFORM_PROJECT needs to be the hash of the actual project
  • platform environment:info id may tell us the environment if the branch name is correct
  • Return to 20220701_from_workshop_finished to make a PR with config.yaml etc.
  • Paul and Jerome will figure out how to find out environment id
  • ddev-redis doesn’t have #ddev-generated in a few different things.
  • Paul will chase the problem with drush -y config-import not having a source directory

@rfay
Copy link
Member Author

rfay commented Jul 11, 2022

Paul on routes:
this is a really good source of info on the routes. Under “Responses”, expand “default” to go get a list of properties and their definitions

@lolautruche
Copy link
Contributor

lolautruche commented Jul 12, 2022

(from discussion in https://platformsh.slack.com/archives/C0JE8AE13/p1657613433824679 )

Paul and Jerome will figure out how to find out environment id

If you haven't already given it an ID via

  • the -e or --environment option
  • the <environment> first argument of some commands (like platform env:del main)
  • passing a full URL to -p
  • PLATFORM_BRANCH environment variable

Then, it will look for the "current environment" via

  • the upstream name of the current git branch
  • the name of the current git branch

And if no environment is found then it'll ask you to choose one.

Also, many commands also need the app or worker name too, if there is more than one (similar process: from the --worker option, --app option, PLATFORM_APPLICATION_NAME variable, or asking).

More details on how this done in the CLI's Selector class.

@rfay
Copy link
Member Author

rfay commented Jul 13, 2022

@lolautruche re: #17 (comment) - it seems to me like there isn't a reliable way to determine PLATFORM_ENVIRONMENT given only token+code, same with PLATFORM_PROJECT, so I'll just have to ask for both. Already doing that in latest commit, but there's a problem with merging config.*.yaml with config.yaml, already knew it would rear its head. Hope to solve in

@rfay
Copy link
Member Author

rfay commented Jul 14, 2022

@rfay
Copy link
Member Author

rfay commented Jul 14, 2022

Most of the things addressed in this PR are now working, even though some of them are shortsighted and will have to be revisited, especially the location of the environment variables PLATFORM_ENVIRONMENT and PLATFORM_PROJECT. We're having to ask for token and both of those, but we should be storing it in the config.platformsh.yaml, which we can't do until ddev does config file merging instead of override, which has an open approved PR, ddev/ddev#3983

If anybody would like to try this out, it would be great. ddev get https://github.com/rfay/ddev-platformsh/tarball/20220630_populate_env

BTW, ddev platform is working nicely, so you can ddev platform ssh for example.

@rfay rfay marked this pull request as ready for review July 14, 2022 22:52
@lolautruche
Copy link
Contributor

PLATFORM_PROJECT can be retrieved using the following command:

platform project:info id

@rfay
Copy link
Member Author

rfay commented Jul 18, 2022

Thanks @lolautruche - that's outstanding, but how in the world can it figure that out without $PLATFORM_PROJECT already being set? Especially with a github project?

$ platform project:info id
Enter a number to choose a project:
  [0] BigFoot Symfony EU-3 (6j4ypl5iendqc)
  [1] d9mariadb (nf35mudfn23bi)
  [2] d9mysql (u2gvy3z2olb5s)
  [3] d9postgres (cojj67jzpv5vk)
  [4] ddev (drume6uf7v66e)
 >
rfay@bf2-web:/var/www/html$ export PLATFORM_PROJECT=6j4ypl5iendqc
rfay@bf2-web:/var/www/html$ platform project:info id
6j4ypl5iendqc

@lolautruche
Copy link
Contributor

Holly Molly 😱 !
I think you have an issue within the container, same issue than for the current environment AFAICT, but I don't know why.
I don't have this issue myself (within or outside the container).

@lolautruche
Copy link
Contributor

Maybe @pjcdawkins can help

@rfay
Copy link
Member Author

rfay commented Jul 18, 2022

I'm pretty sure we decided in slack conversation that there's no way to be assured of knowing the project unless you already know the project. If you look around, it's not in the .platform*. If it's a direct platform-git checkout, then platform cli can look at the git remote and perhaps guess the project from that, but not for a github project like yours.

@lolautruche
Copy link
Contributor

Yes indeed, if it's fresh and not linked (using platform project:set-remote), we won't be able to guess it.
So my suggestion is to:

  1. Ask for the project ID (we may display existing projects using platform projects --count 0
  2. Once we have the project ID, run platform set-remote <project_id>

@pjcdawkins
Copy link

Nitpicking that - it's platform project:set-remote, and it asks for the ID itself if you don't provide one (although it's an autocomplete, not a list)

@rfay
Copy link
Member Author

rfay commented Jul 22, 2022

I was waiting for ddev/ddev#3983 to land to continue here, and it took more work by far than I anticipated, even with a community member starting it off and doing pretty well. But I ended up running with it, then discovering flaws, etc.

Then I returned here and have decided that the approach in this PR is already about as good as it's going to get. It captures the Platform.sh info and puts it in config.yaml (using ddev config). I was hoping it could go into our config.platformsh.yaml instead, but we're replacing that at the time we're asking for it, so it makes a bit of a mess, and having it in the project config.yaml is not necessarily that bad.

So I'm going to pull this and move on to mounts, which should be interesting and less exotic.

@rfay rfay changed the title Beginning work on populating environment variables, for #15 Beginning work on populating environment variables, fixes #15 Jul 22, 2022
@rfay rfay changed the title Beginning work on populating environment variables, fixes #15 Populate environment variables as needed, fixes #15 Jul 22, 2022
@rfay rfay changed the title Populate environment variables as needed, fixes #15 Populate environment variables as needed, fixes #15, fixes #16 Jul 22, 2022
@rfay rfay merged commit 9d194bc into ddev:main Jul 22, 2022
@rfay rfay deleted the 20220630_populate_env branch July 22, 2022 21:18
@rfay rfay mentioned this pull request Jul 22, 2022
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.

3 participants