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

mqtt protocol binding #910

Merged
merged 2 commits into from
Jul 13, 2023
Merged

Conversation

yanmxa
Copy link
Contributor

@yanmxa yanmxa commented Jul 5, 2023

Initial implementation of MQTT binding, using https://github.com/eclipse/paho.golang which supports MQTT V5

Note: This is just a basic implementation; some new features which are brought by MQTT 5.0 and other enhancements will be done in future PRs.

resolved: #901

@yanmxa yanmxa mentioned this pull request Jul 5, 2023
@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 5, 2023

/cc @duglin

@duglin
Copy link
Contributor

duglin commented Jul 5, 2023

@yanmxa thanks. Quick scan looks nice - will review more...

ping @lionelvillard and @embano1 for their review too.

samples/mqtt/README.md Outdated Show resolved Hide resolved
samples/mqtt/README.md Outdated Show resolved Hide resolved
samples/mqtt/sender/main.go Outdated Show resolved Hide resolved
samples/mqtt/sender/main.go Outdated Show resolved Hide resolved
@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 5, 2023

Thanks, @embano1! I modified the test code and doc according to your view. Please feel free to contact me if you have found other issues~

protocol/mqtt_paho/v2/message.go Outdated Show resolved Hide resolved
protocol/mqtt_paho/v2/message.go Outdated Show resolved Hide resolved
protocol/mqtt_paho/v2/message_test.go Outdated Show resolved Hide resolved
protocol/mqtt_paho/v2/protocol.go Show resolved Hide resolved
protocol/mqtt_paho/v2/write_message.go Show resolved Hide resolved
@embano1
Copy link
Member

embano1 commented Jul 5, 2023

Overall looking good @yanmxa We just need some more tests, especially around attribute/extension handling (verification) and writing messages.

@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 6, 2023

Overall looking good @yanmxa We just need some more tests, especially around attribute/extension handling (verification) and writing messages.

@embano1 Sure! I added some unit tests and integration test for protocol and writing messages today. Thanks for your review!

@embano1
Copy link
Member

embano1 commented Jul 6, 2023

Looks good @yanmxa , just one final minor nit

Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

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

GREAT JOB!

Copy link

@morvencao morvencao left a comment

Choose a reason for hiding this comment

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

👍 looks good, thanks @yanmxa

@yanmxa yanmxa force-pushed the br_mqtt_protocol branch 2 times, most recently from e17eed4 to bb1e980 Compare July 7, 2023 13:43
@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 7, 2023

@embano1 Thank you very much for checking the code so carefully, which allows me to avoid similar problems in the future.

@embano1
Copy link
Member

embano1 commented Jul 7, 2023

@embano1 Thank you very much for checking the code so carefully, which allows me to avoid similar problems in the future.

No worries, that's my duty here ;) You're doing great work! Just try to keep the code simple and clean - your future self (and other contributors) will appreciate it ;)

@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 8, 2023

@embano1 Thanks! It was a little complicated about the integration tests with reference to other tests before, I do think too little about simplifying the test. But it should be better now compared to before.

Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

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

Approving, just one minor nit if you feel it's worth the change

test/integration/mqtt_paho_binding/mqtt_test.go Outdated Show resolved Hide resolved
Copy link
Member

@embano1 embano1 left a comment

Choose a reason for hiding this comment

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

LGTM

Feel free to squash and we should be good to go cc/ @duglin

Verified

This commit was signed with the committer’s verified signature.
yanmxa Meng Yan
Signed-off-by: myan <myan@redhat.com>

add protocol and sample files

Signed-off-by: myan <myan@redhat.com>

rollback samples to go1.17

Signed-off-by: myan <myan@redhat.com>

move the protocol to go1.17

Signed-off-by: myan <myan@redhat.com>

add message test

Signed-off-by: myan <myan@redhat.com>

trigger to run the integration test

Signed-off-by: myan <myan@redhat.com>

fixed go mod issue

Signed-off-by: myan <myan@redhat.com>

resolve the review issue

Signed-off-by: myan <myan@redhat.com>

remove the useless comment

Signed-off-by: myan <myan@redhat.com>

add ut for writeMessage

Signed-off-by: myan <myan@redhat.com>

go mod tidy

Signed-off-by: myan <myan@redhat.com>

add intergration test

Signed-off-by: myan <myan@redhat.com>

solve the uncheck error

Signed-off-by: myan <myan@redhat.com>

fix the integration error

Signed-off-by: myan <myan@redhat.com>

fix the integration error

Signed-off-by: myan <myan@redhat.com>

reply the review

Signed-off-by: myan <myan@redhat.com>

simpler tests

Signed-off-by: myan <myan@redhat.com>

remove the nesting

Signed-off-by: myan <myan@redhat.com>

refactor the recevier logic

Signed-off-by: myan <myan@redhat.com>

add a timer for assert loop

Signed-off-by: myan <myan@redhat.com>
@yanmxa yanmxa force-pushed the br_mqtt_protocol branch from 6dabf85 to 3914d99 Compare July 10, 2023 03:06
@yanmxa
Copy link
Contributor Author

yanmxa commented Jul 10, 2023

/cc @duglin @lionelvillard

@@ -0,0 +1,114 @@
package mqtt_paho
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add the copyright?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @lionelvillard ! Done~

@lionelvillard
Copy link
Contributor

Overall it looks good, though I'm not an MQTT expert so there is so much I can review. Please add the copyright header to all files and then I think we can merge this PR.

Verified

This commit was signed with the committer’s verified signature.
yanmxa Meng Yan
Signed-off-by: myan <myan@redhat.com>
@lionelvillard
Copy link
Contributor

looks good. @duglin, ok with you?

@lionelvillard
Copy link
Contributor

@embano1 approved it
LGTM

@lionelvillard lionelvillard merged commit fdcb2d2 into cloudevents:main Jul 13, 2023
@duglin
Copy link
Contributor

duglin commented Jul 13, 2023

sorry - been busy - I trust you guys :-)

@yanmxa yanmxa deleted the br_mqtt_protocol branch July 14, 2023 01:11
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.

Whether to consider adding MQTT protocol binding
6 participants