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

bugfix: do not publish last will when authentication failed #8887

Merged
merged 1 commit into from Sep 5, 2022
Merged

bugfix: do not publish last will when authentication failed #8887

merged 1 commit into from Sep 5, 2022

Conversation

thalesmg
Copy link
Contributor

@thalesmg thalesmg commented Sep 2, 2022

fixes #8886

codeball-ai-writer[bot]
codeball-ai-writer bot previously approved these changes Sep 2, 2022
View reviewed changes
Copy link

@codeball-ai-writer codeball-ai-writer bot left a comment

Choose a reason for hiding this comment

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

Codeball: LGTM! 👍

confidence: 0.99 | [dashboard] | [adjust AI settings]

@thalesmg thalesmg force-pushed the bugfix-will-msg-not-connected branch from afbb975 to c91044d Compare September 2, 2022 20:04
codeball-ai-writer[bot]
codeball-ai-writer bot previously approved these changes Sep 2, 2022
View reviewed changes
Copy link

@codeball-ai-writer codeball-ai-writer bot left a comment

Choose a reason for hiding this comment

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

Codeball: LGTM! 👍

confidence: 0.99 | [dashboard] | [adjust AI settings]

zmstone
zmstone reviewed Sep 2, 2022
View reviewed changes
apps/emqx/test/emqx_broker_SUITE.erl Outdated
payload := <<"should not be published">>
}} ->
ct:fail("should not publish will message")
after 1_000 ->
Copy link
Member

Choose a reason for hiding this comment

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

Maybe use a tp and expect a not sent trace point?

Copy link
Member

Choose a reason for hiding this comment

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

Or wait/assert connection process is gone

Copy link
Contributor Author

@thalesmg thalesmg Sep 2, 2022 •
edited

Choose a reason for hiding this comment

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

Ok, I'll wait for the publisher process to die and check we didn't receive any messages.

Actually, it might be hard to get the connection PID, since it'll die pretty fast during connection 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

✔️

@thalesmg thalesmg force-pushed the bugfix-will-msg-not-connected branch from c91044d to 2e40e35 Compare September 2, 2022 20:58
codeball-ai-writer[bot]
codeball-ai-writer bot previously approved these changes Sep 2, 2022
View reviewed changes
Copy link

@codeball-ai-writer codeball-ai-writer bot left a comment

Choose a reason for hiding this comment

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

Codeball: LGTM! 👍

confidence: 0.98 | [dashboard] | [adjust AI settings]

@thalesmg thalesmg force-pushed the bugfix-will-msg-not-connected branch from 2e40e35 to 51b3a6c Compare September 2, 2022 21:11
codeball-ai-writer[bot]
codeball-ai-writer bot previously approved these changes Sep 2, 2022
View reviewed changes
Copy link

@codeball-ai-writer codeball-ai-writer bot left a comment

Choose a reason for hiding this comment

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

Codeball: LGTM! 👍

confidence: 0.98 | [dashboard] | [adjust AI settings]

@thalesmg thalesmg force-pushed the bugfix-will-msg-not-connected branch from 51b3a6c to e0c0788 Compare September 2, 2022 21:20
codeball-ai-writer[bot]
codeball-ai-writer bot previously approved these changes Sep 2, 2022
View reviewed changes
Copy link

@codeball-ai-writer codeball-ai-writer bot left a comment

Choose a reason for hiding this comment

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

Codeball: LGTM! 👍

confidence: 0.97 | [dashboard] | [adjust AI settings]

@thalesmg thalesmg force-pushed the bugfix-will-msg-not-connected branch from e0c0788 to e0fcf07 Compare September 5, 2022 11:55
codeball-ai-writer[bot]
codeball-ai-writer bot approved these changes Sep 5, 2022
View reviewed changes
Copy link

@codeball-ai-writer codeball-ai-writer bot left a comment

Choose a reason for hiding this comment

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

Codeball: LGTM! 👍

confidence: 0.97 | [dashboard] | [adjust AI settings]

@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 2992993168

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • 33 unchanged lines in 15 files lost coverage.
  • Overall coverage decreased (-0.02%) to 77.379%
Files with Coverage Reduction New Missed Lines %
apps/emqx_authz/src/emqx_authz_schema.erl 1 90.28%
apps/emqx_conf/src/emqx_conf_schema.erl 1 92.31%
apps/emqx_gateway/src/emqx_gateway_schema.erl 1 90.0%
apps/emqx_management/src/emqx_mgmt_api_trace.erl 1 80.0%
apps/emqx_management/src/emqx_mgmt_util.erl 1 13.79%
apps/emqx/src/emqx_authentication_config.erl 1 84.75%
apps/emqx/src/emqx_flapping.erl 1 85.11%
apps/emqx/src/emqx_frame.erl 1 88.41%
apps/emqx/src/emqx_limiter/src/emqx_limiter_schema.erl 1 85.92%
apps/emqx_dashboard/src/emqx_dashboard_monitor.erl 2 68.75%
Totals Coverage Status
Change from base Build 2990318356: -0.02%
Covered Lines: 20606
Relevant Lines: 26630
💛 - Coveralls

@thalesmg thalesmg requested a review from a team September 5, 2022 13:07
@thalesmg thalesmg merged commit 09d640c into emqx:master Sep 5, 2022
@thalesmg thalesmg deleted the bugfix-will-msg-not-connected branch September 5, 2022 13:39
@thalesmg thalesmg mentioned this pull request Sep 8, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zmstone zmstone zmstone left review comments

@ieQu1 ieQu1 ieQu1 approved these changes

@codeball-ai-writer codeball-ai-writer[bot] codeball-ai-writer approved these changes

Assignees
No one assigned
Labels
codeball:approved Codeball
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LWT is sent when a connection is rejected
4 participants
@thalesmg @coveralls @zmstone @ieQu1