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

Have the probe fill in the app id, not the app #1086

Closed
wants to merge 4 commits into from
Closed

Conversation

tomwilkie
Copy link
Contributor

Fixes #1085

@@ -11,7 +11,7 @@ var ErrInvalidMessage = fmt.Errorf("Invalid Message")

// Request is the UI -> App -> Probe message type for control RPCs
type Request struct {
AppID string
AppID string // filled in by the probe on reciept of this request

This comment was marked as abuse.

This comment was marked as abuse.

@2opremio
Copy link
Contributor

2opremio commented Mar 2, 2016

I've done some testing and unfortunately this is not enough to fix #1085

screen shot 2016-03-02 at 7 23 07 pm

There are two additional problems:

  • The first pipe request (GET) is not correctly prefixed. @davkal can you please take a look at this? Also, is there a reason make the second (websocket) request even if the first request didn't succeed?
  • The first pipe request (GET) uses the same endpoint as the second (websocket) request, I discussed this offline with @tomwilkie. This causes a conflict in the nginx configuration since websockets need special treatment (adding an upgrade header in the forwarded request etc ...) . We could have different location rules based on the request type but it's hacky http://stackoverflow.com/questions/8591600/nginx-proxy-pass-based-on-whether-request-method-is-post-put-or-delete

Finally, @tomwilkie , since this is blocking upgrading the service and requires cutting a new release (0.13.1), would you mind re-creating this pull request against a release branch forked from v0.13.0?

@tomwilkie
Copy link
Contributor Author

Sounds like a plan. Let's make this top priority for tomorrow.

@foot is the man you want for ui pipes.

On Wednesday, 2 March 2016, Alfonso Acosta notifications@github.com wrote:

I've done some testing and this is not enough to fix #1085
#1085

[image: screen shot 2016-03-02 at 7 23 07 pm]
https://cloud.githubusercontent.com/assets/2362916/13472358/7153c22c-e0ac-11e5-9a1a-0a1ff0c1a850.png

There are two additional problems:

Finally, @tomwilkie https://github.com/tomwilkie , since this is
blocking upgrading the service and requires cutting a new release (0.13.1),
would you mind re-creating a pull request against a release branch forked
from v0.13.0?


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

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

Successfully merging this pull request may close these issues.

3 participants