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

remove mqtt-c from websockets #14181

Merged
merged 4 commits into from
Jan 23, 2023
Merged

Conversation

underhood
Copy link
Contributor

@underhood underhood commented Dec 23, 2022

Summary

Removes support for MQTT-C library and keeps only new MQTT5 implementation.
This cleans up the code and is prerequisite for next improvements and fixes which are coming.

Test Plan

Behavior should be same as before this PR with mqtt5 switched on

Additional Information
For users: How does this change affect me?

@thiagoftsm
Copy link
Contributor

Hello @underhood ,

Please, rebase your PR to remove conflicts.
The PR still has [WIP] as part of title, is it ready for review?

Best regards!

@underhood
Copy link
Contributor Author

@thiagoftsm not ready for review in summary i write:

Following PRs must be reviewed and merged first (in that order; due to stacked PRs that rely on changes from prior ones):
https://github.com/netdata/netdata/pull/14148
https://github.com/netdata/netdata/pull/14039

we have to merge those 2 first before we can go ahead with this one

@github-actions github-actions bot removed area/collectors Everything related to data collection area/docs collectors/ebpf labels Jan 17, 2023
@underhood underhood changed the title [WIP] remove mqtt-c from websockets remove mqtt-c from websockets Jan 17, 2023
@underhood
Copy link
Contributor Author

@thiagoftsm rebased. Most important in the review here is to check underhood/mqtt_websockets#15 (which this PR will bring in) doesn't accidentally change behavior from now when mqtt5 is true

thiagoftsm
thiagoftsm previously approved these changes Jan 17, 2023
Copy link
Contributor

@thiagoftsm thiagoftsm left a comment

Choose a reason for hiding this comment

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

No issue was observed with different cloud configuration options, LGTM!

@underhood
Copy link
Contributor Author

submodule points to master

@underhood underhood requested review from thiagoftsm and removed request for DShreve2, Ferroin, netdatabot and tkatsoulas January 23, 2023 07:51
@underhood underhood merged commit c2c3876 into netdata:master Jan 23, 2023
@underhood underhood deleted the remove_mqtt_c branch January 23, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/aclk area/build Build system (autotools and cmake).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants