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
MVP Integration w/Pantheon #216
Comments
@beeradb Please scan, but I think this gets us an actionable ticket for a Pantheon MVP. |
The contents of this look good, but I would probably cut a few things out of phase 1 to make it more bite-sized. Those things specifically are:
fwiw, I don’t think this is necessarily even pushing anything back, I think it’s mostly trying to make this huge story achievable in steps. as written, this is a ton of code and would probably keep a dev busy for a couple weeks. The smaller the steps we can make it, the more reviewable and achievable it will be. I also feel like even just the first bullet in the story could be It’s own story for starters, |
Quick response base on a convo in Slack.
If that's the case, I can extract the other points to a wishlist and reference them in existing/new tickets. @rfay @tannerjfco would love a quick sanity check in the next day or so, otherwise I would love to see this actionable by EOW given this could be an issue that takes some time to hammer through while the earlier milestones get completed. |
Updated the acceptance criteria. This is actionable IMHO. |
Need a package/test that precedes this. Specifically
|
Removing from active sprint while we are focusing on the precursor issue #229. |
Here's an initial stab at the CLI commands for an initial Pantheon integration. Global config with interactive promptsValid configI think we're going to need some notion of global config, because we don't want API tokens to be stored anywhere they might actually be committed. I was also hesitant to commit to multi-word (i.e.
Invalid Config
Global config with argumentsValid ConfigWe only have a single value to config, so making it an argument feels natural. That being said, I'm not sure this scales to other implementations. It's hard to design for implementations we haven't seen though :)
Invalid config
Site-Specific configGlobal pantheon config is missing or incorrectIn this case, 'pantheon' is an argument to tell it which import source to use. I imagine import-config will be single command which takes the provider as a plugin, and then calls a method on the provider Invalid argument detected
Global pantheon config is missing or incorrectI would have loved to do something more interactive with the site selection here, but it's hard to build a usable system in a CLI that would work for both 1 site and 100. I think in the end it's probably best to just make the site name input I did not include it in these mock-ups, but I think it probably makes sense to ask if they'd like to run an import directly after configuring. Perhaps just a "you may now run
Running importRunning import with missing/invalid configurationsMissing configuration.
Site not found.
Environment not found.
Running import with valid configurationNeed to ensure the language on this command is suitably scary. It is definitely a destructive action. We also need to make decisions on whether to keep the backups we pull down around or not. I think it probably makes sense to keep them locally and then prompt during import about whether to check for new backups.
|
I spent some time with Elysium today and had an excellent experience. Everything I tried worked as advertised, and casual review of the code shows solid, tested, quality code. Comments:
|
FeedbackThis is specifically related to the current state of the comment here #216 (comment). Global Configurations
Multi-word Versus Hyphenated Words(Ironically, "multi-word" is hyphenated whereas "hyphenated words" is not!)
Site Specific Configurations
|
Responses to @rfay in #216 (comment)
This is absolutely correct. I believe there are factors in play that help mitigate this, but regardless, the risk is still there.
That being said, even with the factors I listed there is absolutely a risk. Guarantees would be fantastic.
Agreed. Elysium was just chosen as a project name so we didn't bike shed. I'd much rather have it be descriptive and meaningful. I've reached out to Josh at Pantheon and asked him about any objections to the name. |
responding to @rickmanelius in #216 (comment)
My thinking was that a single config file with a "provider" key would likely be enough. I think if there's a use case for multiple import sources then having multiple configs might make sense, although we'd have to consider how to handle it in the
This would possibly work for initially setting the value, but what if it needed to be modified? It feels cumbersome to go through this on every site config.
Currently there's no a |
@rickmanelius requested "technical/maintenance considerations that would make a provider plugin either in the main ddev codebase or to extend the ddev codebase untenable. Another factor to consider: how would not including this affect a GUI, which at this point assumes a CLI equivalent/parity": First, nothing makes a provider plugin for Pantheon/Terminus untenable. I think Elysium proves it can be done well. The maintenance issues to weigh are:
Oh and by the way @beeradb is certainly right that the risk is significantly mitigated by the fact that the terminus tool is maintained in public and if we follow it we should know what's going on. On configuration style: It would be ever-so-cool if we could use the Pantheon config that terminus and kalabox already share. I'm not sure that's possible. That would make terminus, kalabox, and ddev interop quite nice. But I sure did like it when I rolled out terminus and it already understood everything based on what I'd done with Kalabox. |
I started reworking everything from above, but ended up backing out of that as I really feel the combination of Global config and ddev importThe only change to these items has been changing Site-Specific configGlobal pantheon config is missing or incorrectIn these examples, I've combined pantheon config with the standard config. Invalid argument detected
Global pantheon config is missing or incorrect
Working config exampleThis shows a combined This also maintains separate YAML files (
|
Overall, this is trending in the right direction. I know this has the ability to get bike-shedded, and I would be willing to accept the last proposal versus arguing over details that may not matter and we can change/refactor based on user feedback. And while I know the linking to private Slack channel's is not ideal in public issue queues, there were some useful summaries worth referencing here https://drud.slack.com/archives/C45DF87JT/p1496871068545215. That said, here are a few things to consider based on the last proposal #216 (comment).
Again, I don't think any of these changes/comments necessitate holding back from moving on creating the first plugin. I'm surfacing these as things to consider from a usability/extendability/support perspective. Of course, user feedback would help us determine which are valid concerns and what items are not important. |
Last minute note before our meeting. Perhaps a way to solve the site versus global configuration is to move to delineate that into two separate functions. Specifically:
|
@rfay @beeradb @tannerjfco and I had a conference call to discuss. The TL;DR is that we have consensus / buy-in to proceed knowing that we're still going to learn things along the way and will likely need to adjust. That said, here are some notes from the conversation. ConfigOverall we like the direction of separating the site-specific and global configuration (even if we don't have an immediate need for global ddev configuration). From a semantic perspective, it's probably best to change the command used to specific hosting provider configurations: "auth" and "connection" were two proposed terms. Summarizing:
ImportFor now, we're going to keep Other future considerations:
This is now actionable. |
One admin item. The issue summary should be updated to reflect the conversation summary as well as other changes from @beeradb's last mocked out functions. Additionally, the testing criteria needs to be updated to reflect the updated naming conventions/command name organization. |
@beeradb I'm going to leave this issue be for now unless you think I can be of service/value on end-user testing or documentation. That said, ready to dive back in as soon as you give me the all clear! |
Technically the PR is closed, but I believed @beeradb wanted this opened as a reminder to address/file any follow-up issues. |
Closing this as #345 is merged and followups have been created. |
What happened (or feature request):
Feature Request
What you expected to happen:
Extend
ddev
to allow:Wishlist of related items (moved to or referenced in other tickets)
Acceptance Criteria
ddev config pantheon
results in an additional set of information placed in .ddev/config.yml namespaced by application type and the provider plugin name to include information needed to communicate with the Pantheon terminus API.ddev import pantheon
available with the ability to specify which environment to pull the database and/or files from.Wishlist related items (moved to other tickets referenced above)
ddev config pantheon
results in the docker-compose.yml files utilizing the pantheon specific images instead of the stock images.ddev import pantheon
for a WordPress captures the production URL and performs a wp-cli search replace to the local database host.Testing Criteria
pantheon
argument results in a docker compose file with Pantheon containers specified.pantheon
argument results in additional data (terminus API credentials) to get stored in configuration.Related source links or issues:
The text was updated successfully, but these errors were encountered: