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

feat(cli): allow specifying the default protocol in config files #1713

Merged
merged 2 commits into from
Jul 10, 2024

Conversation

rpendleton
Copy link
Contributor

What is the current behavior?

Previously, config files couldn't specify the default protocol.

What is the new behavior?

Config files can now specify the default protocol.

The init command has also been updated to prompt for the protocol, and it also recommends a different default port based on the selected protocol.

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

Although I updated the init command to recommend / use a different default port based on the protocol, I didn't update the default port to be based on protocol anywhere else in the app, since I thought that might be too much of a breaking change.

Similarly, if you manually create a config file with a protocol other than mqtt but don't specify a port, it will use the default port of 1883 instead of what was recommended by the init command. We could maybe determine the default port based on the protocol in this case without it being a breaking change (since the protocol config property is new), but I worry whether that will lead to unexpected inconsistencies compared to specifying a protocol with command line flags.

Because of this, I'm not sure whether it actually makes sense for the init script to suggest a different port based on the selected protocol, but I figure it's worth at least discussing.

(And then as an aside, there might be other defaults that should be overridable using a config file, such as websocket path, ca cert, alpn, etc. I didn't have a need for any of those properties in my configuration though, so I only added the protocol for now. Alternatively, it might make sense to use a URI for some of these parameters. That would be a more involved change though.)

@ysfscream ysfscream added enhancement New feature or request feature This pr is a feature CLI MQTTX CLI labels Jul 8, 2024
@ysfscream ysfscream added this to the v1.10.1 milestone Jul 8, 2024
cli/src/configs/common.ts Outdated Show resolved Hide resolved
@rpendleton rpendleton force-pushed the rpendleton/cli-config-protocols branch from c2c8d14 to a1f0816 Compare July 9, 2024 09:23
@ysfscream
Copy link
Member

So many thanks to you! I will merge it.

@ysfscream ysfscream merged commit 43312ac into emqx:main Jul 10, 2024
4 checks passed
@rpendleton rpendleton deleted the rpendleton/cli-config-protocols branch July 14, 2024 01:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLI MQTTX CLI enhancement New feature or request feature This pr is a feature
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants