Pantheon Provider plugin #345

beeradb · 2017-06-21T16:08:48Z

What happened (or feature request):

As per #216 we need pantheon integration. This PR introduces the integration.

At this point, it is feature complete enough to take it for a test drive.

There are three new commands which will be needed:

ddev config-pantheon [token] sets global configuration for talking to the pantheon API. Please see https://pantheon.io/docs/machine-tokens/ for documentation on creating a machine token.
ddev config pantheon extends standard ddev config to account for pantheon specific data.
ddev import imports from a provider. It prints an error if no external provider has been configured.

There are also two new files which get added to the .ddev folder:

import.yaml - This contains pantheon provider-specific data. The goal is that all providers would use this same import.YAML (although with different fields).
provider.env - This is an environment file included by default in docker-compose. It includes provider specific environment variables. In the case of pantheon, Pressflow allows for various configuration options using environment variables, and those get written to provider.env.

How to reproduce it (as minimally and precisely as possible):

Deploy a new site to pantheon. Either Drupal or WordPress will do.
After installation, switch the site to "git" mode and create a backup of the site.
Git clone the repository.
Run ddev config pantheon from the git root directory. Follow configuration prompts.
Run ddev import

Architectural notes

There are a couple decisions here which I'm only somewhat sure I like. I figured I may as well call them out.

Provider is an interface in the ddevapp package. I'm not sure how useful having an interface that only has with only a single (valid) implementation. It seems inevitable to me that if we add more than a single provider we'll need the interface, but it also may change drastically as we add more providers.
There is an empty DefaultProvider struct which implements the provider interface but no-ops all the methods. This seemed preferable to me than always guarding calls to provider methods with if statements to determine if a valid provider config was found or not, but it does feel a bit strange.

Additional musings

provider.env probalby not not be committed. There's nothing particulary secret in it, as even the hash salt value is derived from the json string defining the databases array, but I don't think there's any value in having it until an import occurs, at which point it is automatically written. I'm wondering if we should write a .gitignore to the .ddev directory?
Backups from pantheon are stored in ~/.ddev/pantheon/[sitename]. Each time you run ddev import it will check for the most recent backups in pantheon, but it checks the filesize/filename and doesn't download them again if they already exist. This just speeds things up a bit, though, and the ~/.ddev/pantheon directory can be removed at any time.

rickmanelius · 2017-06-29T18:24:38Z

Testing #345

Reference

Preparation

Download ddev_osx.v0.7-33-ga5397661.tar.gz
Place in /usr/local/bin/ddev
ddev version outputs v0.7-33-ga5397661

Testing

Downloaded a pantheon repo.
Attempted to run ddev config pantheon and was shown an error:
- "2017/06/29 11:59:31 No saved session could be found and the environment variable TERMINUS_API_TOKEN is not set. Please set it to a valid Terminus API Token."
- Might want to actually call out how to create / obtain the token.
Attempted ddev config-pantheon and received the following error:
- "You must provide a Pantheon machine token, e.g. ddev config-pantheon [token]. See https://pantheon.io/docs/machine-tokens/ for instructions on creating a token."
- Might just want to prompt them.
Ran ddev config-pantheon MYTOKEN
- Success!
Re-ran ddev config pantheon
- Configure import environment shows 3 options, but I'm not clear if I just need to type it out.
- Might want to default to dev as a suggestion.
- Putting in an incorrect value (test2) did result in the command prompting me again.
- I see "You may now run ddev start or ddev import"
Run ddev import
- I see the new containers running.
- I see the import of db (6 MB) and files (130 MB)
- I see the "import success, restarting"
- I don't see a URL appear. That would be handy.
- The entire load time from end to end was 1:45 (this included the containers)
Site loads!!!
Re-run ddev import
- I receive no warning that I'm about to nuke my db! :)
- I see no info about a database or files download occuring.
  - After re-reading the pull request thread, I see this is a baked in feature. Nice!
- Reimport worked!
  - I'm assuming it's using a cached copy?
I re-ran a backup job on my live site to create a newer backup.
- The download time was about 50 seconds of the entire 1 minute process.

Spitballing

What if someone wanted to override the default source? Maybe ddev import dev. This is by no means necessary now, but might be a future enhancement.
What if someone just wanted to pull the database and not files?

rickmanelius · 2017-06-29T18:30:57Z

Overall, my reaction is very positive. There is a bit of polish and/or a bit of guidance that can be provided to assist. Example, running the import without a ddev start does spin up the containers, but the URL is not exposed/printed at the end. Little enhancements like that would go a long way.

There is also a bigger need (which I can understand why we would wait for feedback at the technical level first) of getting information ready for our documentation. That in no way impacts the technical review and some of that is included in this PR summary. However, we'd want to make sure this doesn't get neglected.

rickmanelius · 2017-06-29T18:34:41Z

One other error that popped up. When running ddev import a few times in a row, I did hit this once.
Could not perform import: Get https://terminus.pantheon.io:443/api/sites/ac84a9a6a3-0886-4876-a4b7-8b53b5c36dbc4d/environments/live/backups/catalog: EOF

rickmanelius

As stated in #345 (comment) and follow-up comments. Overall this is very positive and moving in the right direction. There is a few use cases to address and areas where additional documentation/instructions can be helpful.

beeradb · 2017-06-30T18:56:15Z

I made the following updates based on feedback from @rickmanelius

Linked to https://pantheon.io/docs/machine-tokens/ in all places where help text/errors are output do to a token not being provided.
Changed the environment selection during config from "please choose an environment" to "Type the name to select an environment to import from"
Added a --skip-confirmation flag + warning prompt to ddev import to alter users of potential data loss.
Added URL output at the end of ddev import

I did NOT do the following:

Prompt user for token value when not providing one via `ddev config-pantheon [token]"

I think prompting works best to avoid flag/argument soup, such as in ddev config where we need to capture a range of values. I'm not very sold on it being a good idea when we have a single value being configured. I'm not completely against it, but consider this some gentle pushing back against the idea :)

rickmanelius · 2017-06-30T20:57:24Z

I'm mid review on the updates. One thing I think that has been surfaced before and I feel strongly will bite us in the ass is that "ddev config-pantheon" and "ddev config pantheon" differ by only a hyphen and yet do completely different things. We discussed things like "auth" or "connection" for the global config. See notes here #216 (comment).

rickmanelius · 2017-06-30T21:02:01Z

When running ddev import, I had the following appear, which I believe is not what is anticipated.
%!(EXTRA string=pantheonsitename)

beeradb · 2017-06-30T21:07:20Z

This is because I updated the warning string without removing the appname parameter. This has been fixed.

Brought to you by

rickmanelius · 2017-06-30T21:12:05Z

So yeah. The latest changes are very helpful. Still a nit about the global config name and the string error cropped up. Beyond that, I think from the end-user perspective this is worthy of a merge, but it does need a technical review and docs. I'm cool with splitting out documentation as a second PR if it would be a big lift (trying to keep commits smaller so that we don't keep long standing WIP).

beeradb · 2017-06-30T21:44:31Z

Agreed. I do hate ddev config-pantheon vs ddev config pantheon. It's something that bothered me when developing, but I figured I'd wait and see what kind of feedback I got.

I'd vote to move it to ddev auth-pantheon since (at this point) that's all it does. We have no guarantee we won't need more config for a future provider, but we can always revisit when we have that information.

rickmanelius · 2017-06-30T21:48:07Z

I'm good with ddev auth-pantheon knowing that it may change and is an accurate placeholder.

rickmanelius · 2017-06-30T21:50:28Z

As discussed in Slack, I'll take a first stab at the documentation.

rickmanelius · 2017-07-03T19:15:01Z

Documentation is being addressed here #369 and is in a "Needs Review" status.

rickmanelius · 2017-07-05T22:22:32Z

@rfay @tannerjfco When you get a chance, I would love to get reviews on this item to get to a consolidated punch list. Thanks!

tannerjfco

I took a first pass through the code. Mostly smaller cleanup stuff and some questions. I'm intending to take this for a test drive and deeper look tomorrow.

tannerjfco · 2017-07-05T23:19:08Z

cmd/ddev/cmd/config.go

+		provider := ""
+
+		if len(args) > 1 {
+			log.Fatal("Invalid argument detected. Please use 'ddev config' or 'ddev config [provider]' to config a site.")


util.Failed

"... to configure a site"

tannerjfco · 2017-07-05T23:26:20Z

cmd/ddev/cmd/import.go

+var ImportCmd = &cobra.Command{
+	Use:   "import",
+	Short: "Import files and database using a configured provider plugin.",
+	Long:  ``,


Needs long help text.

tannerjfco · 2017-07-05T23:30:24Z

cmd/ddev/cmd/import.go

+
+		client := dockerutil.GetDockerClient()
+
+		err := dockerutil.EnsureNetwork(client, netName)


I'm not clear on the intent of this in the PreRun.
util.Failed() would be preferable for error message to the user

This is repeated all over the place, my next commit refactors into a single dockerNetworkPreRun() function for consistency. It also changes the error reporting to use util.Failed()

tannerjfco · 2017-07-05T23:37:00Z

pkg/ddevapp/config.go

-	if err != nil {
-		return err
+		// Create an empty provider.env file to make docker-compose happy.
+		_, err := os.Create(c.GetPath("provider.env"))


do we still need to close an empty file?

do you get an empty file regardless of whether you're using pantheon? what will user be expected to do with the empty file?

You will get an empty provider.env file regardless of whether you are using pantheon or not. Otherwise, docker-compose will throw an error when using the env_file directive. An empty provider.env is now created alongside the docker-compose.yaml, so the user can commit it or ignore it as they choose.

I don't love this, but we need a way to conditionally include environment vars. Open to other suggestions on how to achieve it.

An alternative would be to write a docker-compose.pantheon.yaml that sets the environment variables (or adds an env_file directive). This could potentially be a file managed by ddev.

tannerjfco · 2017-07-05T23:47:18Z

pkg/ddevapp/providerDefault.go

@@ -0,0 +1,46 @@
+package ddevapp


Would it be useful/appropriate to use this no-op default as a form of documentation? I feel like it could be handy to explain the intent for each method, maybe even some basic pseudo code examples.

tannerjfco · 2017-07-06T00:00:08Z

pkg/ddevapp/providerPantheon.go

+	}
+
+	if p.Environment == "" {
+		p.Environment = "dev"


"live" seems like the most appropriate default, but i suppose with pantheon that doesn't exist until a site is close to done (and a credit card has been provided).

Yep! I chose this as the default for that very reason. Dev is the first environment created by default on pantheon, and as such feels like the most consistently available.

I think dev is the right default in every case

tannerjfco · 2017-07-06T00:08:50Z

pkg/ddevapp/providerPantheon.go

+		keys = append(keys, k)
+	}
+	fmt.Println("\n\t- " + strings.Join(keys, "\n\t- ") + "\n")
+	var environmentPrompt = "Type the name to select an environment to import from"


What about numbered options for this prompt?

tannerjfco · 2017-07-06T00:11:56Z

pkg/ddevapp/providerPantheon.go

+		// If we can't read a previous session fall back to using the API token.
+		apiToken := os.Getenv("PANTHEON_API_TOKEN")
+		if apiToken == "" {
+			log.Fatal("No saved session could be found and the environment variable PANTHEON_API_TOKEN is not set. Please use ddev config-pantheon or set a PANTHEON_API_TOKEN. https://pantheon.io/docs/machine-tokens/ provides instructions on creating a token.")


util.Failed

tannerjfco · 2017-07-06T00:16:30Z

pkg/plugins/platform/local.go

-		if err != nil {
-			return err
-		}
+	err := l.AppConfig.WriteDockerComposeConfig()


are we going to 'own' the compose file with this? We may want to consider using the file signature solution randy implemented here if so.

Nope! the write is still conditional, the logic was just moved inside the function.

tannerjfco · 2017-07-06T00:17:12Z

pkg/util/network.go

@@ -30,9 +32,20 @@ func DownloadFile(filepath string, url string) (err error) {
 	if resp.StatusCode != http.StatusOK {
 		return fmt.Errorf("download link %s returned wrong status code: got %v want %v", url, resp.StatusCode, http.StatusOK)
 	}
+	reader := resp.Body
+	if progressBar {


indeed! I felt like a progress bar was a must, as some archives might be quite large, and offering no visual feedback during a 5 gig download seems like a non-starter.

rfay

First round review of test drive:

Yay, things worked great!
Is it worth peeking in ~/.terminus/cache/tokens to get the token with config-pantheon? Or could we just use that cache instead of creating .ddev/pantheonconfig.json?
I really can't go with competing ddev config pantheon and ddev config-pantheon commands. Pretty sure they just frustrate customers and will be the an object of frustration. I think I've seen this discussed already. Looks like there's a plan to use authenticate-pantheon as the command?
I wasn't clear about what the starting situation was. Should I be doing this in an empty directory? In a created ddev directory? Rick's docs say that I need to git clone the upstream; that seems like more work that we'd expect people to undertake, but they are going to need a clone at some point.
Error "You must provide a Pantheon machine token, e.g. ddev config-pantheon [token]." should not include markdown backticks - it's plain text.
Why does it go through the whole start (container creation) process just to do it again after the import?
ddev import seems like a problematic addition. I think it will be confusing.
Don't forget to add post-import capabilities after pre/post goes in.
I'm not actually all that comfortable yet with what we're doing with settings.php, will have to review the nginx-php-fpm PR and think through other possibilities.

I'l review the code and the fpm PR a little later.

beeradb · 2017-07-06T18:38:33Z

Is it worth peeking in ~/.terminus/cache/tokens to get the token with config-pantheon? Or could we just use that cache instead of creating .ddev/pantheonconfig.json?

We could probably peek if we wanted. Terminus does support multiple logins in that folder, though, which could raise some concerns over which we use (just randomly pick one? try multiple to see if they are active? what if it's an old account or an expired token?). We'd need to decide what the behavior should be.

Personally, I'm fairly against writing anything to the terminus token cache.

I really can't go with competing ddev config pantheon and ddev config-pantheon commands.

Agreed. config-pantheon has been renamed to auth-pantheon in the latest push.

Why does it go through the whole start (container creation) process just to do it again after the import?

Containers need to be started to perform the additional import. After the import is done, we need to inject DB credentials and other settings via environment vars (since settings files are committed on pantheon). Doing this requires us writing a provider.env (or perhaps a docker-compose.pantheon.yaml as Tanner suggested earlier) and a restart of the site for those variables to take effect.

ddev import seems like a problematic addition. I think it will be confusing.

Totally open to alternative suggestions. We could re-use import-db and import-files and break it into a two-step process, but I was worried about overloading those commands with vastly different behaviors.

tannerjfco

Needs some work for WordPress sites. Imports seem to be successful, but site produces 500 error after start due to invalid configuration file (for this environment).

It seems like we should be able to set the PANTHEON_ENVIRONMENT env var to have the credentials read from env vars. We'll additionally need to define these variables:

AUTH_KEY
SECURE_AUTH_KEY
LOGGED_IN_KEY
NONCE_KEY
AUTH_SALT
SECURE_AUTH_SALT
LOGGED_IN_SALT
NONCE_SALT

We'll also need to ensure a search-replace happens after import.

If I manually configure wp-config.php and perform a search-replace, the wordpress site seems to work as expected from there.

I was not testing w/ the right web tag. This feedback is invalid.

tannerjfco · 2017-07-06T20:07:49Z

Why does it go through the whole start (container creation) process just to do it again after the import?

Containers need to be started to perform the additional import. After the import is done, we need to inject DB credentials and other settings via environment vars (since settings files are committed on pantheon). Doing this requires us writing a provider.env (or perhaps a docker-compose.pantheon.yaml as Tanner suggested earlier) and a restart of the site for those variables to take effect.

Do we need to read values from the imported database (or some other downloaded asset) in order to set these env vars? Just wondering if we can set these vars before the import occurs, so that we don't have to restart.

ddev import seems like a problematic addition. I think it will be confusing.

Totally open to alternative suggestions. We could re-use import-db and import-files and break it into a two-step process, but I was worried about overloading those commands with vastly different behaviors.

It makes sense to me to re-use the separated import commands. Dealing with locally stored assets, their functionality isn't all that impressive, but I think they become quite a bit more impressive and useful handling syncing from upstream environments. I also think developers should be able to import just the database or files, regardless of provider. It's not uncommon for developers to only want a db dump, and to use something like stage_file_proxy to avoid pulling files (this becomes necessity for sites w/ a large volume of assets). I don't feel like it would be unreasonable for these to accept provider as argument, e.g. ddev import-db pantheon. I'm not opposed to a command that does both db and files, but I think any import commands should work both locally or with a provider.

…is created

…ing the default provider

rfay · 2017-07-17T19:09:54Z

Per @beeradb I rebased this for testing purposes.

rfay · 2017-07-17T19:29:07Z

@beeradb There's no sign that the TestPantheon* tests have been running during circle tests (or during mine on Windows). I don't know why this is. I think it's important to resolve before pull. See https://circleci.com/gh/drud/ddev/1348 for example.

Edit: It turns out that the tests in config_test.go are also being ignored.

… Newconfig

beeradb · 2017-07-17T22:33:39Z

With this last test run passing and @rfay giving the green light on windows, it's time to hit the button!

beeradb added work in progress needs docs needs automated tests labels Jun 21, 2017

beeradb force-pushed the pantheon-provider branch from 2c27601 to b9985fc Compare June 28, 2017 21:19

rickmanelius requested review from rfay, rickmanelius and tannerjfco June 29, 2017 19:25

rickmanelius suggested changes Jun 29, 2017

View reviewed changes

tannerjfco reviewed Jul 6, 2017

View reviewed changes

rfay requested changes Jul 6, 2017

View reviewed changes

tannerjfco previously requested changes Jul 6, 2017

View reviewed changes

beeradb force-pushed the pantheon-provider branch from ad4a048 to 006ebc4 Compare July 6, 2017 19:48

beeradb added 14 commits July 17, 2017 13:04

[pantheon-provider] Update to latest pantheon hash

00d4d2d

[pantheon-provider] make GetGlobalDdevDirectory ensure the directory …

d0ac1fa

…is created

[pantheon-provider] Fix import cycle from moving GetGloblaDdevDir()

cbca6d5

[pantheon-provider] Small fixes + improvements from code review feedback

c9a911c

[pantheon-provider] Allow provider to be reconfigured

fc1cd3b

[pantheon-provider] remove existing import files if it exists when us…

b97e48e

…ing the default provider

[pantheon-provider] ensure provider cannot be a blank string

61c37e0

[pantheon-provider] improved documentation for test dependencies

13f5024

[pantheon-provider] add context to error from archive extraction

a036425

[pantheon-provider] Don't add newline after prompt output

d3bfb8d

[pantheon-provider] swap order of test defers to work on windows

e15d474

[pantheon-provider] chmod settings files before writing

7527748

[pantheon-provider] Check error on os.Chmod

a96a484

[pantheon-provider] PANTHEON_API_TOKEN -> DDEV_PANTHEON_API_TOKEN

9828640

rfay force-pushed the pantheon-provider branch from 00a7fbe to 9828640 Compare July 17, 2017 19:09

beeradb added 3 commits July 17, 2017 13:46

[pantheon-provider] call m.Run from within TestMain()

d4dc35b

[pantheon-provider] don't exit until teardown completes

9cf461b

[pantheon-provider] Fix failing tests by properly setting defaults in…

a58c4d8

… Newconfig

beeradb removed needs docs needs automated tests work in progress labels Jul 17, 2017

beeradb merged commit be965c8 into ddev:master Jul 17, 2017

rickmanelius mentioned this pull request Jul 18, 2017

v0.9.0 Release Checklist #377

Closed

6 tasks

This was referenced Jul 18, 2017

Add configuration flags for ddev pull - --ignore-db, ignore-files, etc. #405

Closed

Add a ddev push <provider> command #406

Closed

Provide a warning if settings.local.php is not included #407

Closed

MVP Integration w/Pantheon #216

Closed


		client := dockerutil.GetDockerClient()

		err := dockerutil.EnsureNetwork(client, netName)

Pantheon Provider plugin #345

Pantheon Provider plugin #345

Conversation

beeradb commented Jun 21, 2017 • edited

What happened (or feature request):

How to reproduce it (as minimally and precisely as possible):

Related source links or issues:

beeradb commented Jun 28, 2017 • edited

beeradb commented Jun 28, 2017 • edited

Architectural notes

Additional musings

rickmanelius commented Jun 29, 2017

Reference

Preparation

Testing

Spitballing

rickmanelius commented Jun 29, 2017

rickmanelius commented Jun 29, 2017

rickmanelius left a comment

Choose a reason for hiding this comment

beeradb commented Jun 30, 2017

rickmanelius commented Jun 30, 2017

rickmanelius commented Jun 30, 2017

beeradb commented Jun 30, 2017 • edited

Brought to you by

rickmanelius commented Jun 30, 2017

beeradb commented Jun 30, 2017 • edited

rickmanelius commented Jun 30, 2017

rickmanelius commented Jun 30, 2017

rickmanelius commented Jul 3, 2017

rickmanelius commented Jul 5, 2017

tannerjfco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rfay left a comment • edited

Choose a reason for hiding this comment

beeradb commented Jul 6, 2017

tannerjfco left a comment

Choose a reason for hiding this comment

tannerjfco commented Jul 6, 2017

rfay commented Jul 17, 2017

rfay commented Jul 17, 2017 • edited

beeradb commented Jul 17, 2017

beeradb commented Jun 21, 2017 •

edited

beeradb commented Jun 28, 2017 •

edited

beeradb commented Jun 28, 2017 •

edited

beeradb commented Jun 30, 2017 •

edited

beeradb commented Jun 30, 2017 •

edited

rfay left a comment •

edited

rfay commented Jul 17, 2017 •

edited