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

chore: Remove MQTT Page and Native Subscriptions Components from C2 UI #6970

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

blegesse-w
Copy link
Member

Closes granite#3226

This PR addresses part of issue 3226 by removing the Native Subscriptions/MQTT page and any components exclusively used by it from the codebase. The associated subscriptions tests were already removed in a previous PR. With this change, we're continuing the cleanup of deprecated MQTT-related features as customers no longer use them

Checklist

Authors and Reviewer(s), please verify the following:

  • A PR description, regardless of the triviality of this change, that communicates the value of this PR
  • Well-formatted conventional commit messages that provide context into the change
  • Documentation updated or issue created (provide link to issue/PR)
  • Signed CLA (if not already signed)
  • Feature flagged, if applicable

@blegesse-w blegesse-w requested review from a team as code owners October 28, 2024 16:51
@blegesse-w blegesse-w changed the title Chore: Remove MQTT Page and Native Subscriptions Components from C2 UI chore: Remove MQTT Page and Native Subscriptions Components from C2 UI Oct 28, 2024
Copy link
Contributor

@wdoconnell wdoconnell left a comment

Choose a reason for hiding this comment

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

Hi @blegesse-w Thanks for doing this - I had two comments:

  1. I'll revalidate when I can spin up remocal, but I believe we need to still expose the UI elements that reference the telegraf MQTT plugin (not part of the C2 native subscriptions feature).
  2. Before closing the issue (i.e. after this PR is merged) there is still a little more cleanup to do (removing the feature flag in ConfigCat and IDPE).

src/writeData/constants/contentTelegrafPlugins.ts Outdated Show resolved Hide resolved
src/writeData/utils/updateTelegrafPlugins.mjs Outdated Show resolved Hide resolved
cypress/e2e/cloud/deepLinks.test.ts Outdated Show resolved Hide resolved
@blegesse-w blegesse-w force-pushed the chore/remove_mqtt_components branch 2 times, most recently from 113bff0 to 8a1822e Compare November 1, 2024 17:07
)}
/>
)}
{CLOUD && (
Copy link
Member Author

Choose a reason for hiding this comment

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

not sure why this wasn't put behind the subscriptionsUI ff. 🤔

wdoconnell
wdoconnell previously approved these changes Nov 13, 2024
Copy link
Contributor

@wdoconnell wdoconnell left a comment

Choose a reason for hiding this comment

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

Content looks good, and I tested in remocal as well. Thanks for doing this!

Before merging, could you please remove the whitespace changes in the two plugin files?

@@ -77,3 +77,4 @@
## Value supported is int, float, unit
# [[inputs.mqtt_consumer.topic.types]]
# key = type

Copy link
Contributor

Choose a reason for hiding this comment

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

I would git checkout main -- [this file] to revert any change -- it's just whitespace but we don't want history that suggests a change was made here since we are not altering availability of the telegraf plugin.

@@ -197,7 +197,7 @@ sensors,site=CLE,version=v1,device_name=device5 temp=390,rpm=45.0,ph=1.45
- when [[inputs.internal]] is set:
- payload_size (int): get the cumulative size in bytes that have been received from incoming messages
- messages_received (int): count of the number of messages that have been received from mqtt

Copy link
Contributor

@wdoconnell wdoconnell Nov 13, 2024

Choose a reason for hiding this comment

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

Same comment here re: space in this file we aren't changing.

@wdoconnell
Copy link
Contributor

Rerunning monitor-ci tests: they failed because of a remocal deployment failure that is likely unrelated to this PR.

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.

2 participants