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.

Sign up for GitHub

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

Jump to bottom

feat: nolocal flag support for bridges #737

Merged
merged 14 commits into from
May 12, 2022
Merged

feat: nolocal flag support for bridges #737

merged 14 commits into from
May 12, 2022

Conversation

oldrich-s
Copy link
Contributor

@robertsLando robertsLando changed the title Fixes "Nolocal flag for bridges is not respected" fix: nolocal flag for bridges is not respected Apr 26, 2022
robertsLando
robertsLando requested changes Apr 26, 2022
View reviewed changes
lib/handlers/subscribe.js Show resolved Hide resolved
lib/client.js Outdated Show resolved Hide resolved
@robertsLando
Copy link
Member

robertsLando commented Apr 26, 2022 •
edited
Loading

Also please add a unit test that covers this and fix broken tests

@robertsLando
Copy link
Member

@gnought Opinions on this one?

@coveralls
Copy link

Pull Request Test Coverage Report for Build 2232406743

  • 0 of 13 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.2%) to 99.736%
Totals Coverage Status
Change from base Build 2231280775: -0.2%
Covered Lines: 808
Relevant Lines: 808
💛 - Coveralls

@coveralls
Copy link

coveralls commented Apr 27, 2022 •
edited
Loading

Pull Request Test Coverage Report for Build 2312799555

  • 13 of 13 (100.0%) changed or added relevant lines in 3 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage decreased (-0.09%) to 99.824%
Totals Coverage Status
Change from base Build 2231280775: -0.09%
Covered Lines: 808
Relevant Lines: 808
💛 - Coveralls

robertsLando
robertsLando approved these changes May 12, 2022
View reviewed changes
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for this PR @oldrich-s ! Is it ready to merge?

robertsLando
robertsLando reviewed May 12, 2022
View reviewed changes
test/bridge.js Outdated Show resolved Hide resolved
@oldrich-s
Copy link
Contributor Author

LGTM. Thanks for this PR @oldrich-s ! Is it ready to merge?

I have just added qos 1 test (just to be sure that persistence works as expected). But otherwise I believe it is ready to be merged.

robertsLando
robertsLando requested changes May 12, 2022
View reviewed changes
test/bridge.js Outdated Show resolved Hide resolved
robertsLando
robertsLando requested changes May 12, 2022
View reviewed changes
lib/client.js Show resolved Hide resolved
test/bridge.js Show resolved Hide resolved
oldrich-s and others added 4 commits May 12, 2022 12:20
@oldrich-s @robertsLando
Add reference to MQTTv5
82a634c 
Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>
@oldrich-s @robertsLando
Add reference to bridge mode enabled
e0ca896 
Co-authored-by: Daniel Lando <daniel.sorridi@gmail.com>
@oldrich-s
Add qos 2 to bridge unit test
ae8dabf
@oldrich-s
Merge remote-tracking branch 'refs/remotes/origin/main'
3efcf52
robertsLando
robertsLando approved these changes May 12, 2022
View reviewed changes
Copy link
Member

@robertsLando robertsLando left a comment

Choose a reason for hiding this comment

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

LGTM. Good to merge?

@oldrich-s
Copy link
Contributor Author

LGTM. Good to merge?

yeps 👍

@robertsLando robertsLando changed the title fix: nolocal flag for bridges is not respected feat: nolocal flag support for bridges May 12, 2022
@robertsLando robertsLando merged commit f82b8f8 into moscajs:main May 12, 2022
@robertsLando robertsLando mentioned this pull request May 12, 2022
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@robertsLando robertsLando robertsLando approved these changes

@gnought gnought Awaiting requested review from gnought

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oldrich-s @robertsLando @coveralls