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

Separation of local vs remote lowercase-C contexts (for cd, prefix, run/local, etc) #1752

Open
bitprophet opened this issue May 14, 2018 · 6 comments
Labels
Milestone

Comments

@bitprophet
Copy link
Member

@bitprophet bitprophet commented May 14, 2018

(Cannot find an old ticket for this :( thought there was one...)

Fabric 1 had with cd: and with lcd: and similar dual APIs for users that needed to track context-y state (cwd, prefixes list etc) differently for the remote vs local ends.

Invoke only has the one local context and doesn't care.

Fabric 2 inherits from Invoke but failed to grow distinct remote-vs-local APIs before 2.0 (besides the much earlier implementation of "move Invoke's run to Fabric 2's local", which presents its own set of problems around "what kind of Context am I dealing with right now and what methods does it have?").

This needs solving and I assume in the obvious fashion, i.e. growing new API members (named explicitly either after local or remote) and the appropriate state storage (on Connection or in config).

This will sadly be a backwards compatibility break (insofar as with c.cd() currently changes both ends and will revert to changing only one) but the software's only been out a week and I think we can argue that this was a bug. Otherwise it'll be the shortest lived major version number ever 😐

This is strongly related to #1686 which is for the run-vs-local config divergence.

@rpkilby
Copy link
Contributor

@rpkilby rpkilby commented May 14, 2018

Hi @bitprophet - were you thinking of #1637?

@bitprophet bitprophet changed the title Separation of local vs remote lowercase-C contexts (for cd, prefix, etc) Separation of local vs remote lowercase-C contexts (for cd, prefix, run/local, etc) May 14, 2018
@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Jun 22, 2018

#1637 got rebased into #1755, FTR. And I still have to doublecheck whether that approach works for me or not!

@rpkilby
Copy link
Contributor

@rpkilby rpkilby commented Jun 22, 2018

Yep. There is still some work to be done on #1755, but it's in a state where it and it's sibling PR for invoke can be reviewed/discussed.

@bitprophet
Copy link
Member Author

@bitprophet bitprophet commented Aug 22, 2018

#1858 brings up one possible angle on the run-vs-local side of things, namely adding a local to invoke.Context so that the two APIs resemble one another more closely.

I think we still want to reject this approach, as it sullies Invoke and doesn't entirely solve the issue (we're still gonna have problems with things like cd, or any new API members either side of the equation adds later).

I also do still think (at this point in time, months later) that #1755's approach may be the only sensible way forward, tho I have to reread it and give a harder think on any other obvious APIs that come to mind. But having the two sides of the conversation as discrete API objects seems hard to beat for explicitness. It's just gonna be hard to wrangle re: backwards compatibility (& if put out in a backwards compat fashion it'll introduce extra confusion; however given there's already a lot of confusion around this......)

@rpkilby
Copy link
Contributor

@rpkilby rpkilby commented Nov 17, 2018

Had an idea.. it might be possible to maintain compatibility at the executor level. In #1755, FabExecutor constructs a Context or a Connection, depending on whether or not the call is to a LocalTask or a Task. It could also check for regular Invoke Tasks (which users will already be using) and construct a backwards compatible connection instead, maintaining the old API (and issue a deprecation warning).

The question then is whether or not it's worth the added complexity and whether or not this behavior will confuse users. IDK if this might be too "clever". Worth trying - can always dump the changes.

@DylanYoung
Copy link

@DylanYoung DylanYoung commented Sep 28, 2019

I'd suggest replacing the "Connection" object with a multi-context: FabricContext.

By default FabricContext would delegate all Context API calls to its remote (fabric Connection) and only by calling FabricContext.local would we have access to the local (invoke Context). Calls to remote could also be explicit (FabricContext.remote.cd). You could implement convenience methods like lcd on the FabricContext which auto-delegate to the local, but I think this just clutters the API.

This is a very similar proposal to #1755, but drops the necessity to support two separate APIs. The FabricContext is still a valid invoke Context as is the fabric Connection. It's just that the context now contains two contexts and either automatically or manually chooses which to use for a specific invocation.

Moreover, this opens up the possibility of supporting multiple remotes in a single Context without hacky modifying of 'host' dynamically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.