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

new: add ability for caller to get back received raw *nats.Msg to a channel #56

Merged
merged 3 commits into from
May 29, 2019

Conversation

aaslamin
Copy link
Contributor

@aaslamin aaslamin commented May 26, 2019

Issue: https://github.com/aporeto-inc/aporeto/issues/1274

This PR introduces a new option to the NATS pubsub Publish method: NATSOptRespondToChannel

This option allows clients to receive the response *Publication to a channel they configure when using the NATS request-reply messaging pattern. This is particularly useful in situations where the client is publishing something within an HTTP handler and they need to block and wait for a reply so they can use the response message to craft an appropriate HTTP response (e.g. Aporeto automations handling webhooks!).

Sample usage:

	responseCh := make(chan *Publication)
	errorCh := make(chan error)

	ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second)
	defer cancel()

	go func() {
		if err := p.pubsub.Publish(pub, bahamut.NATSOptRespondToChannel(ctx, responseCh)); err != nil {
			errorCh <- err
		}
	}()

	select {
	case response := <-responseCh:
		fmt.Printf("received a message: %+v\n", response)
	case err := <-errorCh:
		fmt.Printf("received an error: %+v\n", err)
	}

Other things

  • This option is backwards compatible with existing logic.
  • New unit tests added ✅

👀Review: @primalmotion @t00f

…hannel they provide when using the request-reply mode

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
@primalmotion
Copy link
Contributor

I think it would be more symetrical to return a publication in response

@aaslamin
Copy link
Contributor Author

I think it would be more symetrical to return a publication in response

Yeah, you're right because the only thing you can publish right now is a Publication. Good catch, I will make that change.

@aaslamin
Copy link
Contributor Author

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "56",
    "commit-sha": "30acc338353db9accc27c0657ce1d0c31e9ecc32"
  }
]

@codecov
Copy link

codecov bot commented May 26, 2019

Codecov Report

Merging #56 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #56      +/-   ##
==========================================
+ Coverage   88.27%   88.35%   +0.07%     
==========================================
  Files          28       28              
  Lines        2107     2121      +14     
==========================================
+ Hits         1860     1874      +14     
  Misses        220      220              
  Partials       27       27
Impacted Files Coverage Δ
pubsub_nats_options.go 100% <100%> (ø) ⬆️
pubsub_nats.go 51.85% <100%> (+3.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 59d7918...2b1f06e. Read the comment docs.

…r Publish API - when using the Request-Reply pattern, the response is expected to be a Publication as well

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
…sponse could not be decoded into a Publication when using the NATSOptRespondToChannel option

Signed-off-by: Amir Aslaminejad <amir.a@aporeto.com>
@aaslamin
Copy link
Contributor Author

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "56",
    "commit-sha": "2b1f06eca20102727c3d66247fdc2fc1cb2e50e9"
  }
]

@aaslamin
Copy link
Contributor Author

aaslamin commented May 28, 2019

Update @primalmotion

  • A response will be sent to the response channel if and only if the raw *nats.Msg could be decoded into a Publication, otherwise an error is returned - 201a564
  • Another unit test added to cover the new behaviour - 2b1f06e

EXPECT().
RequestWithContext(gomock.Any(), pub.Topic, expectedPublishData).
Return(&nats.Msg{
Data: []byte("this cannot be decoded into a publication!"),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

👀

Copy link
Contributor

@primalmotion primalmotion left a comment

Choose a reason for hiding this comment

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

LGTM. please run a test of the backend with this

Edit: oh you did
Edit 2: oh no you did not
Edit 3: actually we need a full ft in here

@aaslamin
Copy link
Contributor Author

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "",
    "component": "bahamut",
    "pr-id": "56",
    "commit-sha": "2b1f06eca20102727c3d66247fdc2fc1cb2e50e9"
  },
  {
    "project": "",
    "component": "backend",
    "pr-id": "408",
    "commit-sha": "6f3c24caaae0e91a09c4257d22a410299978dccd"
  }
]

@aporeto-bot
Copy link

/build - automatically fired by gogo with following PRs and commit SHAs v1.0.0

[
  {
    "project": "receive-publish-response",
    "component": "bahamut",
    "pr-id": "56",
    "commit-sha": "2b1f06eca20102727c3d66247fdc2fc1cb2e50e9"
  },
  {
    "project": "receive-publish-response",
    "component": "backend",
    "pr-id": "408",
    "commit-sha": "6f3c24caaae0e91a09c4257d22a410299978dccd"
  }
]

@primalmotion
Copy link
Contributor

ft failed on a unrelated unstable test. merging

@primalmotion primalmotion merged commit c56cba4 into master May 29, 2019
@primalmotion primalmotion deleted the add-ability-to-receive-raw-message branch May 29, 2019 17:45
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