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

Added custom headers support for WebSocket connection #505

Closed
wants to merge 14 commits into from
Closed

Added custom headers support for WebSocket connection #505

wants to merge 14 commits into from

Conversation

vit21ik
Copy link
Contributor

@vit21ik vit21ik commented Mar 22, 2018

#502

Minor fix that allow use custom headers in request using WebSockets :

Added new parameter "customHeaders" to MqttConnectOptions
Pass this param to WebSocketHandshake
Added new custom headers to handshake request
Please make sure that the following boxes are checked before submitting your Pull Request, thank you!

  • [*] You have signed the Eclipse ECA
  • [*] All of your commits have been signed-off with the correct email address (The same one that you used to sign the CLA)
  • [*] If This PR fixes an issue, that you reference the issue below. OR if this is a new issue that you are fixing straight away that you add some Description about the bug and how this will fix it.
  • If this is new functionality, You have added the appropriate Unit tests.

@vit21ik
Copy link
Contributor Author

vit21ik commented Mar 22, 2018

What going on?

The pull request did not pass Eclipse validation.
My profile - https://accounts.eclipse.org/users/vvlasiuk4xs

@jpwsutton jpwsutton self-requested a review March 26, 2018 10:02
@jpwsutton
Copy link
Member

Hi @vit21ik, Thanks for your contribution, it looks quite useful! I'll review your changes today, but you'll need to update your commit messages to include the Signed-off-by message with the email address used to sign the ECA: https://wiki.eclipse.org/ECA before we can accept them.

Thanks - James

Copy link
Member

@jpwsutton jpwsutton left a comment

Choose a reason for hiding this comment

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

Would it be possible for you to move the example from the MQTTv3.md file into the project Wiki Please?

Also, in MqttConnectOptions, please could you rename customHeaders to customWebsocketHeaders along with the Getter and Setter. Other than that it looks good!

@jpwsutton jpwsutton self-assigned this Mar 26, 2018
@@ -81,7 +81,7 @@
private String[] serverURIs = null;
private int mqttVersion = MQTT_VERSION_DEFAULT;
private boolean automaticReconnect = false;

private Properties customHeaders = null;

Choose a reason for hiding this comment

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

Is there a reason to use the type Properties instead of a more generic Map<String,String>? Properties implement the very old and obsolete Dictionary interface and new code should not necessarily use it if no old java properties file is read.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can see that Properties used for sslClientProps

 private Properties sslClientProps = null;

From the beginning I used Map, but changed to Properties for uniformity with existed sslClientProps.

@vit21ik
Copy link
Contributor Author

vit21ik commented Apr 30, 2018

  1. Updated MQTTv3.md
  2. Renamed customHeaders to customWebSocketHeaders
  3. Added custom headers to mqtt v5

@jpwsutton
Copy link
Member

Thanks for making the updates @vit21ik, please could you update your commit messages to include the Signed-off-by message with the email address used to sign the ECA: https://wiki.eclipse.org/ECA, Otherwise we won't be able to accept them.

@jpwsutton jpwsutton added the Waiting for contributor This issue or PR is blocked, awaiting more information or an action from the contributor. label May 23, 2018
vitalii-temy and others added 7 commits June 7, 2018 17:03
# Conflicts:
#	org.eclipse.paho.client.mqttv3/src/main/java/org/eclipse/paho/client/mqttv3/MqttConnectOptions.java
#	org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/MqttConnectionOptions.java
# Conflicts:
#	org.eclipse.paho.client.mqttv3/src/main/java/org/eclipse/paho/client/mqttv3/MqttConnectOptions.java
#	org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/MqttConnectionOptions.java
Signed-off-by: Vitalii <vitalii.vlasiuk@temy.co>

# Conflicts:
#	org.eclipse.paho.client.mqttv3/src/main/java/org/eclipse/paho/client/mqttv3/MqttConnectOptions.java
#	org.eclipse.paho.mqttv5.client/src/main/java/org/eclipse/paho/mqttv5/client/MqttConnectionOptions.java
Signed-off-by: Vitalii <vitalii.vlasiuk@temy.co>
…nto develop

Signed-off-by: vvlasuk@gmail.com <vvlasuk@gmail.com>
@vit21ik
Copy link
Contributor Author

vit21ik commented Jun 8, 2018

Hi guys
I resolved merge conflict, but:

  1. The Travis CI build failed - what is going on? looks after I merged develop to forked branch, build failed.
  2. I updated my commit message - added: -author="Author Name v....k@gmail.com" (because usually I worked with another email by default). You can see that my icon on commits also changed. Anyway I see that "The following users do not have valid ECAs". Despite https://accounts.eclipse.org/users/vvlasiuk4xs/eca. Could someone help me with this?

@davidgraeff
Copy link

You should squash all commits into one and sign that. The IP validation doesn't look at only the last commit, but all commits have to be signed with the correct e-mail.

@vit21ik
Copy link
Contributor Author

vit21ik commented Jun 8, 2018

Created new pull request instead of this with correct Eclipse validation (sorry, I cannot manage with validation in current)
#554

@jpwsutton
Copy link
Member

Closed as this was resolved in PR #554

@jpwsutton jpwsutton closed this Jun 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Waiting for contributor This issue or PR is blocked, awaiting more information or an action from the contributor.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants