feat: added mqttClientId parameter with a defualt value #295
Conversation
VaishnaviNandakumar
requested review from
Tenischev,
derberg and
asyncapi-bot-eve
as code owners
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
|
@Tenischev Please have a look. |
README.md
Outdated
| |disconnectionTimeout|Only for MQTT. The completion timeout in milliseconds when disconnecting. The default disconnect completion timeout is 5000 milliseconds.|No| `5000` | | ||
| |completionTimeout|Only for MQTT. The completion timeout in milliseconds for operations. The default completion timeout is 30000 milliseconds.|No| `30000` | | ||
| |asyncapiFileDir| Path where original AsyncAPI file will be stored.|No| `src/main/resources/api/` | | ||
| |mqttClientId| Only for MQTT. Provides the client identifier for the MQTT server.|No| guest | |
There was a problem hiding this comment.
- I suggest to move it one row higher. so, all "only mqtt" parameters will be together.
- Are you sure that you need to change formatting of all rows?
- I suggest to point priority of choosing parameter more clearly e.g. "Overrides the value of the client identifier for the MQTT server, if it is set in the AsyncAPI, otherwise it provides a default value"
There was a problem hiding this comment.
Hi @Tenischev
- Updated the text and position in
README.mdandpackage.json. - No it doesn't require formatting of all rows. In
if-elif-elseapproach, the first check is to see ifparams.mqttClientIdexists. When a default value is set formqttClientIDinpackage.json, this condition would always be true which is why I removed it and directly put the default value assignment inapplication.yml.
There was a problem hiding this comment.
@Tenischev Hi, please take a look at the updated changes.
package.json
Outdated
| @@ -129,6 +129,11 @@ | |||
| "description": "Only for Kafka. Add type information to the message header", | |||
| "default": "true", | |||
| "required": false | |||
| }, | |||
| "mqttClientId": { | |||
| "description": "Only for MQTT. Provides the client identifier for the MQTT server.", | |||
There was a problem hiding this comment.
same point about description here
| {% if server.binding('mqtt') and server.binding('mqtt').clientId %}clientId: {{server.binding('mqtt').clientId}}{% endif %} | ||
| {% if server.binding('mqtt') and server.binding('mqtt').cleanSession | isDefined %}cleanSession: {{server.binding('mqtt').cleanSession}}{% endif %} | ||
| {% if server.binding('mqtt') and server.binding('mqtt').lastWill %}lastWill: | ||
| {% if server.binding('mqtt') and server.binding('mqtt').clientId and (params.mqttClientId == 'guest') %} |
There was a problem hiding this comment.
I see the problem if in AsyncAPI server.binding('mqtt').clientId = provider (example), but user would like to intentionally change it to guest via parameter.
There was a problem hiding this comment.
I see. I had an approach where I made use of an if-elif condition like this -
{% if params.mqttClientId %}
clientId : {{ params.mqttClientId }}
{% elif if server.binding('mqtt') and server.binding('mqtt').clientId %}
clientId: {{ server.binding('mqtt').clientId }}
{% else %}
clientId: defaultValue
{% endif %}
But this requires the default value to be removed from package.json and I wasn't sure if it was better. What do you think?
Tenischev
changed the title
|
🎉 This PR is included in version 0.29.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
mqttClientIdwith a default valueguestwas added topackage.json.Related issue(s)
Resolves #293