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

v2 style @hosts or similar #1772

Closed
bitprophet opened this Issue May 21, 2018 · 9 comments

Comments

Projects
None yet
2 participants
@bitprophet
Copy link
Member

bitprophet commented May 21, 2018

TODO

  • Seek out any earlier tickets about this and merge them
  • Update upgrade doc
  • Append changelog
  • Add regular docs
  • Tests
  • Implementation
  • Real world test

Background

  • v1 had a basic @hosts decorator which slapped a single string or list of strings onto the decorated function object, which execution machinery looked for as a hint for default target list.
    • There was a lot of hooplah around "merging" CLI --hosts, task @hosts, global host list configs, etc.
    • I'm not sure we can avoid all of that this time around but I think we can avoid most of it...right now the only way to "set a host list" outside of the library use case is --hosts, and my scope right now is to just allow setting that sort of thing at a task level instead of a global CLI level.
  • Its use case was aimed at small, lightweight fabfiles that cared very strongly about a specific environment, e.g. "this migrate task always runs against admin@db.example.com:1234" or "this deploy task defaults to one call against www with default user/port, and another against app2 with user webadmin"
  • The "correct" (scales up better, etc) way to do this is to separate definition of "what to do" (tasks/recipes) from "where to do it" (hosts/groups, but this more DSL-esque use case is still quite valid at the low end, and tackling that more "grown up" use case is appropriate for redoing @roles.

Implementation ideas

  • End result should be similar to --hosts, aka Executor's determination of 'which tasks to run with which inputs' needs to go from "they said to run deploy" to "they said to run deploy against three specific Connections"
  • Where --hosts deals solely with strings (because CLI) we have the opportunity (and responsibility!) here to allow full Connection setup params - host strings are absolutely a convenience only.
  • The exact Python-level values that go into this decorator/argument are up in the air:
    • Minimum is same as CLI, i.e. "host strings"
    • But we want to avoid that being anything but shorthand, so also need a way of carting around user/host/port tuples or similar
    • This scales way up to capturing any Connection parameters and invites the idea of a separate "struct" like object such as a Host, which is basically a "partial Connection".
    • See comment(s) below for detailed thought train on this.
  • There's an open question here about whether this should live in its own @hosts decorator or if it should be an argument to a Fabric variant on Invoke's @task, eg @task(hosts=['foo', 'bar'])
    • Up til now I've not seen a great argument for the more-decorators pattern, and it is more complex to implement (have to make 100% sure they all can go in any order and are all signature/docstring/etc preserving, etc)
    • In very early v1, we needed it as a separate decorator because there was no @task - which is not the case nowadays.
    • We can always add "specialized" decorators later (if someone comes up with a good argument for the pattern) in a backwards compatible fashion.
    • Counterargument: until now we've avoided needing a Fabric version of @task and adding one can lead to newbie confusion ("halp why does my @task get mad about hosts=xxx??? what do you mean I have to change the import?")
    • Counter-counterarguments:
      • it's going to be impossible to avoid some of that sort of thing (dueling imports) over time
      • could update tutorial to encourage use of the Fabric version of the decorator from the get-go, even if not using its new args right away
      • users used to Invoke will be used to the "just use @task" pattern and might not like having to branch out to N-decorators

@bitprophet bitprophet added the Feature label May 21, 2018

@bitprophet

This comment has been minimized.

Copy link
Member Author

bitprophet commented May 21, 2018

Another big open question here is whether to wait for a larger, more holistic solution that tackles @roles/etc too. For example, if we did that, we could perhaps change to a @targets/@task(targets=xxx) API, instead of starting with @hosts, adding @roles/@groups, etc.

For now my inclination is to get something usable out now; if we end up starting to pile-on layers of junk, we can start deprecating & cleaning up for a 3.0 release later on.

@bitprophet bitprophet added this to the 2.1 milestone May 21, 2018

@bitprophet

This comment has been minimized.

Copy link
Member Author

bitprophet commented May 21, 2018

Re: exact format of data given to this decorator/arg:

  • Connection is currently defined as a binding of cxn params and config data (brought at instantiation time) and network state (brought at Connection.open time, aka the first time a networky method is run).
    • So we can't just have the user slap Connection instances into a @hosts decorator (or any other lazily-evaluated config structure about where-to-connect) or it may lead to confusion and state problems
  • At a base level, we need a way of carting around the connection params by themselves, e.g. a host string or user/host/port three-tuple
    • Though arguably, everything besides the actual connection state falls under this - what if you wanted to specify that some deploy target is not only user foo on IPv6-enabled host 2001:db8::ff00:42:8329 at port 2222, but that it should have a custom connect_timeout or connect_kwargs.key_filename?
  • One way might be to define a new class, Host, that is explicitly a container for connection data and can be passed around, copied, etc and only ever matters as input to a Connection.__init__ (eg overload that 1st posarg a second time so it can be hostname, host string, or Host. Not great but that's backwards compat for ya.)
  • Alternately, we could try harder to reuse Connection for this in a way that guards against accidental reuse - e.g. have Executor check connection state of these inputs to make sure they're not already connected
    • But this feels dirty to me and still has edge cases like "what if it was a previously flown booster connected Connection?"
    • While safeguards are nice, a solution that won't crap out errors some % of the time is preferable, I think even if it means adding a bit of extra mental overhead.
    • If we make Host the "beefed up host string" of the API, users with simple cases don't need to really know about it until they need it. (E.g. leave it out of the tutorial or put it in some "advanced use cases" 2nd tutorial.)
  • Argument in favor of no-Host: since it largely needs to include the entire Connection.__init__ API, now that API is in two places, has to get tracked twice, can accidentally deviate, etc. Long term maintainer pain.
  • Another: we already have existing .clone patterns for this class hierarchy and this could mostly be another wrinkle on it - even if (accidentally) given a "used" connection, if we always clone-on-use, fewer problems.
    • Unless users start expecting that the literal Connection given will be the one used, which is its own big can of worms...
  • Argument against reusing Connection: mental overhead/overload seems bad. Are these objects lazy, or not? What do you mean I can use it directly or give it to @hosts? (which should then be @connections??)

Taking a step back, what if we separated the APIs such that Connection is phrased as an "expression" of a given Host? This feels like it makes semantic sense; keeps most API params in one place (Host); etc. However it's also not backwards compatible so can't jump on it right away, but it's food for thought.

Another idea: is there anything wrong with just using dicts here, aka "the thing you'd shove into Connection(**kwargs)"? Since that's currently all it would be for, why obfuscate it behind a dedicated-but-tiny class/struct? And this further ties into the presumed eventual "define these dicts/whatevers in YAML/JSON/TOML/etc" roles stuff - in that case, too, we'd be going from dict to Host and then basically back to kwargs. Why jump straight to the extra middleman?

@bitprophet

This comment has been minimized.

Copy link
Member Author

bitprophet commented May 21, 2018

Starting in on the API specific DDD for this now and my brain is telling me to just go with @task(targets=[...]) up front:

  • hosts as a term already feels slightly off in a hard-to-quantify-fashion (basically, as soon as it departs from just host strings?)
  • It's already going to be an arguably messy heterogenous list-type due to accepting strings plus kwarg dicts, and we know we're almost surely ending up with some form of labelled saved roles/groups eventually.
    • Counter: implementation-wise, it gets dicier, having to guess if a given token is a shorthand host string, a role/group label name, or etc.
    • Especially when we look at kwarg dicts. Is a given dict for instantiating a Connection or a Group (granted, makes less sense for Group anyways...)?

Eh. Let's stick with hosts for now. It also has the benefit of being that much closer to what v1 users are familiar with.

@bitprophet

This comment has been minimized.

Copy link
Member Author

bitprophet commented May 21, 2018

One minor argument in favor of the separate-decorator pattern: stuffing a bunch of documentation into a single :param block in the docstring for @task feels a little grody. Not enough to undo the decision but just putting it out there.

@Meekohi

This comment has been minimized.

Copy link

Meekohi commented Jun 28, 2018

Just dropping some feedback in -- the @hosts decorator and simplicity of fabfile.py was a huge reason we started using Fabric originally. Was pretty jarring to find it removed in v2 with no obvious replacement. Most of my files were just full of very simple commands like:

env.roledefs = {
  'primary': ['oneboxA'],
  'secondary': ['oneboxB', 'spinworker1', 'spinworker2', 'spinworker3']
}

@parallel
@roles('primary', 'secondary')
def pull():
  with cd("meekohi-projA"):
    run("git pull")

@parallel
@roles('primary')
def migrate():
  with cd("meekohi-projA"):
    run("rake db:migrate")

and it seems now I need to explicitly create connections and then pass them around to each function (or stuff boilerplate connection logic into each one)? The other alternative of trying to remember which hosts to use on the command line is a bit too complicated. So, looking forward to seeing this coming back and would focus on simplicity for this common use case.

@bitprophet

This comment has been minimized.

Copy link
Member Author

bitprophet commented Jul 2, 2018

@Meekohi Yup for now, I'm leaning towards "reinstate the common case first" specifically to help folks in your boat get over to 2.x faster.

Also, if you haven't already, you may want to follow tickets like #4 / #1594 (for roles and host definitions) and make sure you look over the upgrading doc, which has tables linking most other applicable "missing feature" type tickets.

bitprophet added a commit that referenced this issue Jul 12, 2018

bitprophet added a commit that referenced this issue Jul 12, 2018

Stub tests for new Fabric-level Task/task re #1772
Includes ConnectionCall test which currently fails
@bitprophet

This comment has been minimized.

Copy link
Member Author

bitprophet commented Jul 12, 2018

Put out Invoke 1.1 which makes a bunch of minor-moderate changes required for Fabric-level @task/Task. And merged the feature branch for this. Doing some final sanity tests before I pop out Fabric 2.2 with this in it, I think. No real point sitting on it while I hit other tickets.

@bitprophet

This comment has been minimized.

Copy link
Member Author

bitprophet commented Jul 12, 2018

Not only did I do a quick literal sanity test, I also did some wackadoodle private experimentation which didn't leverage @task(hosts=...) so much as all of the intermediate guts I had to write to get to this point. Kinda neat: https://twitter.com/bitprophet/status/1017555060476145664

There's more work to do but I don't think it's anything that needs to hold up the release, it'd affect already public APIs in Invoke anyways (Executor vs Call vs Task). So I'll probably pop it out by EOW.

@bitprophet

This comment has been minimized.

Copy link
Member Author

bitprophet commented Jul 13, 2018

Going up on PyPI as 2.2 momentarily.

@bitprophet bitprophet closed this Jul 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.