Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Flux Adapter preparation #2211

Merged
merged 2 commits into from
Jul 2, 2019
Merged

Flux Adapter preparation #2211

merged 2 commits into from
Jul 2, 2019

Conversation

squaremo
Copy link
Member

@squaremo squaremo commented Jul 1, 2019

  • makes API changes so that all methods are available over HTTP, including NotifyChange (used for relaying webhooks, for example) /cc @alanjcastonguay
  • adds some notes about what can be moved out of this repo and into weaveworks/flux-adapter

This (modulo passing tests and review) can be merged now, in principle, because it doesn't alter what is served over the Upstream connection -- it just adds to the HTTP API. But I am going to make it a draft until I've Actually Tried that.

Fixes #2210.

@squaremo squaremo added this to the decouple-from-weavecloud milestone Jul 1, 2019
@ellieayla
Copy link
Contributor

ellieayla commented Jul 1, 2019

Looks like you missed two api.UpstreamServer references in tests during the great renaming.

remote/mock_test.go:    ServerTestBattery(t, func(mock api.UpstreamServer) api.UpstreamServer { return mock })
remote/rpc/rpc_test.go: wrap := func(mock api.UpstreamServer) api.UpstreamServer {```

@squaremo squaremo force-pushed the flux-adapter-removals branch from 048c005 to 585daca Compare July 2, 2019 10:06
@squaremo
Copy link
Member Author

squaremo commented Jul 2, 2019

This PR also updates the Upstream connection so it connects to the v11 endpoint. This was introduced as wee while ago to support rollout progress with ListServicesWithOptions, but back-filled in the (versioned) RPC clients so that it would work with older fluxd too.

Usually there's a gap necessary between introducing a new version and rolling out a Flux release that uses it, so that the service part of it can be rolled out in Weave Cloud -- so probably what happened is that, since it worked with the back-filled RPC client, the latter part of the rollout was forgotten.

In any case, I've now Actually Tried It with

  • a flux daemon connecting to Weave Cloud (v11 endpoint)
  • a flux daemon pointed at the flux adapter, itself connected to Weave Cloud

and both of these work. Specifically, I was looking for all the UI elements to work OK, and for notifications to be propagated to the service.

It should also work fine with justinbarrick/fluxcloud, because that accepts websocket connections at any path.

@squaremo squaremo marked this pull request as ready for review July 2, 2019 11:10
Copy link
Member

@hiddeco hiddeco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code changes look solid, and given #2211 (comment) there is enough evidence it works :-)

squaremo added 2 commits July 2, 2019 12:25
This commit moves the Upstream methods (Ping, etc., available only
over RPC) to the Server v11 interface, and abandons the Upstream
interface. This is so that any client, including
weaveworks/flux-adapter, can access these methods.

It also updates the upstream registration point to v11 -- an
incidental but related change.
@squaremo squaremo force-pushed the flux-adapter-removals branch from 585daca to cf79228 Compare July 2, 2019 11:26
@squaremo squaremo merged commit 0eb0b4b into master Jul 2, 2019
@squaremo squaremo deleted the flux-adapter-removals branch July 2, 2019 13:59
squaremo added a commit to weaveworks/flux-adapter that referenced this pull request Jul 2, 2019
fluxcd/flux#2211 moved the api.Upstream methods into api.Server,
and implemented them for the HTTP API server and client. That means we
no longer need a shim to stub the extra methods on top of an HTTP
client.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Upstream API to regular API
3 participants