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

[Host] Optionally include IConsumerContext and/or CancellationToken in consumer method invocation #224

Conversation

EtherZa
Copy link
Contributor

@EtherZa EtherZa commented Mar 29, 2024

Added support for optionally including IConsumerContext and/or the CancellationToken in the consumer method invocation when supplying a type and method name.

As the CancellationToken is tied to the life of the message lock, it should be readily available in the method to prematurely terminate processing when the lock is lost.

Supplying the IConsumerContext as an argument, as opposed to constructor or setting injection, allows for a single consumer instance where appropriate.

public class SomeConsumer
{
  public async Task MyHandleMethod(SomeMessage msg, IConsumerContext consumerContext, CancellationToken cancellationToken)
  {
    // handle the msg
  }
}

Please note that the order of the parameters is not important.


IConsumer and IRequestHandler have not been modified so as to avoid creating a breaking change. Implementation would be trivial should there be appetite to change the signatures.

Though not direct solutions to #220 and #223, the submission addresses both issues for non-IConsumer/IRequestHandler implementations.

@EtherZa
Copy link
Contributor Author

EtherZa commented Mar 30, 2024

@zarusz there appears to be an issue with the SonarCloud secret

 The format of the analysis property sonar.token= is invalid

@zarusz
Copy link
Owner

zarusz commented Mar 30, 2024

Yes, I believe secrets are not passed to branch builds which are from external repo clones. I will have a look when I get a chance.

@zarusz
Copy link
Owner

zarusz commented Mar 30, 2024

@EtherZa I've changed the actions workflow some on master, can you please rebase your changes and push again?

@zarusz zarusz added this to the 2.3.0 milestone Apr 1, 2024
@zarusz
Copy link
Owner

zarusz commented Apr 1, 2024

@EtherZa can you please rebase against master once more and ideally squash all your changes into 1 commit?
I am trying to tweak the build workflow so that SonarCloud and secrets for integration test infra are shared for PR coming from cloned repos. Unfortunately, that might require a couple of back and forth to get right.

I have looked at your changes - looks good!

@EtherZa EtherZa force-pushed the feature/inject-consumer-context-and-cancellation-token-into-handler branch 2 times, most recently from 91e64e1 to 80983a4 Compare April 1, 2024 12:20
@EtherZa
Copy link
Contributor Author

EtherZa commented Apr 1, 2024

@zarusz - No worries, I've rebased, squashed and pushed.

@EtherZa EtherZa force-pushed the feature/inject-consumer-context-and-cancellation-token-into-handler branch from 80983a4 to 9ee1411 Compare April 2, 2024 00:53
@zarusz
Copy link
Owner

zarusz commented Apr 2, 2024

@EtherZa can you rebase once more?
I've pushed some more tweaks to build workflow.

I see that sonar cloud integrates nicely already.

After that I am happy to merge.

@EtherZa EtherZa force-pushed the feature/inject-consumer-context-and-cancellation-token-into-handler branch from 9ee1411 to a8eea57 Compare April 2, 2024 06:39
@EtherZa
Copy link
Contributor Author

EtherZa commented Apr 2, 2024

Rebased and pushed without the unused variable.

@EtherZa EtherZa force-pushed the feature/inject-consumer-context-and-cancellation-token-into-handler branch 2 times, most recently from 5eab7f1 to 6de2929 Compare April 2, 2024 08:02
@EtherZa
Copy link
Contributor Author

EtherZa commented Apr 2, 2024

Both SonarCloud issues resolved and pushed.

@EtherZa EtherZa force-pushed the feature/inject-consumer-context-and-cancellation-token-into-handler branch from 6de2929 to 4e30201 Compare April 2, 2024 08:17
Copy link

sonarqubecloud bot commented Apr 2, 2024

@zarusz zarusz merged commit 2469e7f into zarusz:master Apr 2, 2024
2 checks passed
@zarusz
Copy link
Owner

zarusz commented Apr 2, 2024

This is now merged. Thanks for your contribution @EtherZa!
I will add this to the upcoming 2.3.0 release (soon).

@EtherZa
Copy link
Contributor Author

EtherZa commented Apr 2, 2024

Thank you!

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

Successfully merging this pull request may close these issues.

2 participants