Skip to content
This repository has been archived by the owner on Dec 12, 2019. It is now read-only.

Document Pantheon Import role. #14

Merged
merged 4 commits into from
Dec 7, 2015

Conversation

wizonesolutions
Copy link
Contributor

Given that I haven't written this yet, this is obviously not merge ready. I wanted to open a PR early though and get your thoughts on the "API" for the role. Anything you see as a UX WTF that could be better? Keeping in mind it's a v1.

Checklist:

  • For variables remaining in Vlad, prefix with vlad_

@philipnorton42
Copy link
Contributor

This looks great! 👍
Disclaimer: I don't use Pantheon so I'm not sure how useful my input here would be. However, one point I would make is for the pantheon_import_site_include_db setting. I would say that making this either distinct from the "other" db importer or to tie them together in some way would be better than having a bunch of caveats. For example, we could get a panteon task to drop the database into the correct place for the Vlad auto db import to pickup.

@dixhuit
Copy link
Contributor

dixhuit commented Oct 5, 2015

Initial thoughts (apologies if a little brief - I'm juggling a fair bit at the mo):

Currently, and going forward, we're prefixing any new variables added to Vlad with vlad_ in order to prevent namespacing issues now that we're limbering up to use more & more userland code from Ansible Galaxy. If (see below) all of these new vars are to be included within Vlad itself then I'd like to see them adopt the same convention.

pantheon_import_site_email and pantheon_import_site_password are in no way Vlad specific, they're not really even "site import specific". I'd like to see them renamed and for the (empty) defaults to be defined in https://github.com/hashbangcode/ansible-role-pantheon-cli even if that Galaxy role doesn't directly use them yet:

  • pantheon_cli_account_email
  • pantheon_cli_account_password

Can we do something with Ansible Vault here to prevent having to store any passwords as plain text?

Can more of your proposed functionality also live in the existing Pantheon CLI role? For example, the wrapper code that imports a database, and then we look at Vlad then implementing that in a more specific way (e.g. pass in existing Vlad vars such as db details)? I'm keen for more of these non-Vlad-specific features be just that, non-Vlad-specific, so that we can better abstract things and make more use of Galaxy and the community around it. It may not be practical but I think we need to aim for that first to find out for sure.

@dixhuit
Copy link
Contributor

dixhuit commented Oct 5, 2015

Sorry - so as not to come across as just critical, this is a good idea for sure and something that Pantheon users will surely find useful. I'm only a minor Pantheon user at present but I'm already fighting the urge to automate a bunch of what their CLI has to offer - partly because I just can't remember all the extra commands :)

@wizonesolutions
Copy link
Contributor Author

@philipnorton42 Yep, exact plan is to drop the DB into {{ aux_synced_folder }}/db_io/halt_destroy/vladdb.sql.gz. I got Pantheon CLI to change --to-directory to simply --to so I can save it there directly via Pantheon CLI.

@danbohea Thanks for the feedback! It wasn't too critical. This is what I need to get it done in a way that will work well with Vlad.

Making specifically those variables part of https://github.com/hashbangcode/ansible-role-pantheon-cli is a good idea. Would I still document them here?

I can look into Ansible Vault. At first glance, it seems like it's more for when you want to put passwords directly into your variable file. Since this would be user-level config, it might be a bit much to ask them to use Ansible Vault. I think, though, that they could use it without us having to make any changes. A simple explanation or example of that might be good to link to in the docs.

Hmm...in terms of more commands living in pantheon_cli, how do you figure that would work? How do we make it properly idempotent? We would need more variables than doing it in Vlad, at least. e.g. to tell it where it should save the DB, whether it should overwrite it, and so on. It can be done. But I dunno. That might be best even as a role of its own. The way I see https://github.com/hashbangcode/ansible-role-pantheon-cli is that it installs the CLI such that it's available. But that might just be because of how it was initially implemented. Did you have plans to flesh it out more later?

@dixhuit
Copy link
Contributor

dixhuit commented Oct 6, 2015

The existing Pantheon CLI role was intended to initially just provide the tool but I always hoped that it could be fleshed out further - I just haven't had the chance to properly look into it (you beat me to it!). Providing that the additional functionality isn't Vlad-only, I see no reason why that repo can't accept PRs that extend it further, especially in terms of common use cases, time savers and just not having to remember the whole range of commands/options. Some of this conversation should probably take place over there once we've got a better handle on the what/how.

@dixhuit
Copy link
Contributor

dixhuit commented Oct 6, 2015

Making specifically those variables part of https://github.com/hashbangcode/ansible-role-pantheon-cli is a good idea. Would I still document them here?

Document them in the README of the Galaxy role. We've still got to figure out how best to connect/aggregate docs from separate Galaxy roles but it's absolutely the way to go in terms of maintaining docs for each role.

@wizonesolutions
Copy link
Contributor Author

Maybe we can link to the role docs from here. Perhaps a new top-level
section called Roles or Extensions. So that we avoid not just having them
use the Galaxy role variables directly.
6. okt. 2015 03:10 skrev "Dan Bohea" notifications@github.com:

Making specifically those variables part of
https://github.com/hashbangcode/ansible-role-pantheon-cli is a good idea.
Would I still document them here?

Document them in the README of the Galaxy role. We've still got to figure
out how best to connect/aggregate docs from separate Galaxy roles but it's
absolutely the way to go in terms of maintaining docs for each role.


Reply to this email directly or view it on GitHub
#14 (comment)
.

@dixhuit dixhuit mentioned this pull request Oct 6, 2015
@wizonesolutions
Copy link
Contributor Author

Alright, so I am thinking more about this today. For the time being, I'm going to mostly continue how I was (and prefix those variables with vlad_), and we could look at moving it in a follow-up (or you can refuse to merge my PR and pressure me into it once I submit one) :P

If I don't do this, I'll get caught up in architecture thinking even longer and probably wind up doing nothing. I see some ways in which putting more stuff in pantheon-cli could work, but it isn't as clear-cut and potentially not as easy to configure.

Basically: I want to do the MVP first, to see if anyone even cares, and then refine as I get more comfortable with everything.

@wizonesolutions
Copy link
Contributor Author

Renamed variables in proposed docs. Leaving open until there is actual code written against these.

@wizonesolutions
Copy link
Contributor Author

Ah, btw, @philipnorton42, is it technically possible to set the value of db_import_up if the user doesn't configure it? To override the existing default? If so, I could have fewer caveats for it to work because I could just set it dynamically when they didn't.

@dixhuit
Copy link
Contributor

dixhuit commented Oct 9, 2015

Fair enough - I know where you're coming from in terms of "do it this way, at least it's a way, better than not happening at all". Like you say, we can always pull it apart and put it back together again in terms of architecture as a later task. Good luck!

BTW, you can turn your code into threads if you sign up for Hacktoberfest before submitting any more PRs.

@philipnorton42
Copy link
Contributor

@wizonesolutions Yes, I think it's possible to set the value via a condition during the ansible execution. If you get stuck then let me know and I'll try and post some code snippets :)

@wizonesolutions
Copy link
Contributor Author

Getting there! Sneak preview:

TASK: [pantheon_import_site | include_vars {{ item }}] ************************
ok: [192.168.100.100] => (item=all.yml)

TASK: [pantheon_import_site | pantheon_import_site | Log in to Pantheon] ******
changed: [192.168.100.100]

TASK: [pantheon_import_site | pantheon_import_site | code | Check if docroot already exists] ***
skipping: [192.168.100.100]

TASK: [pantheon_import_site | pantheon_import_site | db | Check if database backup already exists] ***
ok: [192.168.100.100]

TASK: [pantheon_import_site | pantheon_import_site | db | Ensure vlad_aux/db_io directories exist] ***
ok: [192.168.100.100] => (item=db_io)
ok: [192.168.100.100] => (item=db_io/halt_destroy)

TASK: [pantheon_import_site | pantheon_import_site | db | Download latest database backup from Pantheon] ***
changed: [192.168.100.100]

# ...

TASK: [mysql | mysql_import | Import database(s) from default source file(s)] ***
changed: [192.168.100.100] => (item=vladdb)

@philipnorton42
Copy link
Contributor

@wizonesolutions 👍 Awesome :)

@wizonesolutions
Copy link
Contributor Author

Also, not going to allow importing code in v1 of this. I doubt many people will use that. Even I won't; I generally have a mirror repo that I then push to Pantheon. If people ask for importing code (cloning the Pantheon repo) it wouldn't be super-hard to add, because we can find out that info via Pantheon CLI. But I just want to get something released, so I'm gonna cut that.

Will finish this up tonight or Friday and submit a PR to the Vlad repo!!! (and remove the DISCUSSION tag on this one, for docs).

@dixhuit
Copy link
Contributor

dixhuit commented Oct 22, 2015

Cool. Looking forward to the PR :)

@wizonesolutions wizonesolutions changed the title DISCUSSION: Document Pantheon Import role. Document Pantheon Import role. Oct 23, 2015
@wizonesolutions
Copy link
Contributor Author

This PR's ready now!

Code: hashbangcode/vlad#292

@wizonesolutions
Copy link
Contributor Author

@philipnorton42 don't forget this one :)

@wizonesolutions
Copy link
Contributor Author

@philipnorton42 @danbohea Pantheon's in — any reason you're holding off on this one?

@dixhuit
Copy link
Contributor

dixhuit commented Nov 25, 2015

Sorry - been waaaay too busy. The only reasons I was holding off were:

  1. It was a bit broken (now fixed I understand, have yet to test)
  2. I was wondering if we can tweak the role name a bit (shorter hopefully) - that would have a knock on effect for var naming.

Still very busy so can't go into to detail right now but will be sure to carve out some time this week.

@wizonesolutions
Copy link
Contributor Author

Makes sense.
25. nov. 2015 15:21 skrev "Dan Bohea" notifications@github.com:

Sorry - been waaaay too busy. The only reasons I was holding off were:

  1. It was a bit broken (now fixed I understand, have yet to test)
  2. I was wondering if we can tweak the role name a bit (shorter
    hopefully) - that would have a knock on effect for var naming.

Still very busy so can't go into to detail right now but will be sure to
carve out some time this week.


Reply to this email directly or view it on GitHub
#14 (comment)
.

@dixhuit
Copy link
Contributor

dixhuit commented Dec 6, 2015

OK, quick heads up to say that I've just renamed pantheon_import_site to pantheon_import before any of this functionality get's formerly documented and included in a tagged release. This change is now in dev.

@wizonesolutions Would you mind updating your PR to reflect this?

Replaced `vlad_pantheon_import_site` variables with `vlad_pantheon_import`.
@wizonesolutions
Copy link
Contributor Author

@danbohea OK, how's that?

On Sun, Dec 6, 2015 at 1:56 PM, Dan Bohea notifications@github.com wrote:

OK, quick heads up to say that I've just renamed pantheon_import_site to
pantheon_import before any of this functionality get's formerly
documented and included in a tagged release. This change is now in dev
hashbangcode/vlad@a6c6460
.

@wizonesolutions https://github.com/wizonesolutions Would you mind
updating your PR to reflect this?


Reply to this email directly or view it on GitHub
#14 (comment)
.

@dixhuit
Copy link
Contributor

dixhuit commented Dec 7, 2015

  1. It's shorter (but still specific enough) which as you know makes things easier to type, read and discuss.
  2. This variable no longer has to be called this: vlad_pantheon_import_site_site (reminds me of "Organic Groups groups" :) )
  3. The specificity of that namespace was perhaps unnecessary, a "site" and aspects of it are pretty much the only things you'd be expecting to import from Pantheon within the context of Vlad. Plus, pantheon_import feels more future proof. Don't get me wrong, I appreciate the care taken to ensure specificity, I'm just keen to ensure we don't over do it where we don't need to.
  4. With this functionality now in dev it felt like we were against the clock to finalise namespacing in order to minimise future disruption to users by fiddling with things after a tagged release (e.g. variable names changing).

Does that make sense to you?

@wizonesolutions
Copy link
Contributor Author

Sorry, "that" meant my PR update :) I edited it.
7. des. 2015 11:16 skrev "Dan Bohea" notifications@github.com:

  1. It's shorter (but still specific enough) which as you know makes
    things easier to type, read and discuss.
  2. This variable no longer has to be called this:
    vlad_pantheon_import_site_site (reminds me of "Organic Groups groups"
    :) )
  3. The specificity of that namespace was perhaps unnecessary, a "site"
    and aspects of it are pretty much the only things you'd be expecting to
    import from Pantheon within the context of Vlad. Plus, pantheon_import
    feels more future proof. Don't get me wrong, I appreciate the care taken to
    ensure specificity, I'm just keen to ensure we don't over do it where we
    don't need to.
  4. With this functionality now in dev it felt like we were against the
    clock to finalise namespacing in order to minimise future disruption to
    users by fiddling with things after a tagged release (e.g. variable names
    changing).

Does that make sense to you?


Reply to this email directly or view it on GitHub
#14 (comment)
.

@dixhuit
Copy link
Contributor

dixhuit commented Dec 7, 2015

LOL, yeah, that looks about right! Will merge now.

dixhuit added a commit that referenced this pull request Dec 7, 2015
@dixhuit dixhuit merged commit 78f8a00 into hashbangcode:dev Dec 7, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants