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

Introduced new property 'url' in MQTTWebsocketTransport to accept custom url. #461

Merged
merged 2 commits into from
May 12, 2018

Conversation

mendirattanishant
Copy link
Contributor

Inject URL in MQTTWebsocketTransport.

AWS IoT gives self signed URLs. Using the url property, user can set the url property directly instead of using MQTTWebsocketTransport method endpointURL framing it for you using port, tls, etc.

Copy link
Contributor

@jcavar jcavar left a comment

Choose a reason for hiding this comment

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

Thanks @mendirattanishant. Added few comments.

Also, can we add some tests around this?

/** url an NSString containing the websocketurl
* defaults to @""
*/
@property (strong, nonatomic) NSString *url;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not NSURL and nil by default?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the changes. Let me know if they look fine.

@@ -34,8 +36,15 @@ - (instancetype)init {
- (void)open {
DDLogVerbose(@"[MQTTWebsocketTransport] open");
self.state = MQTTTransportOpening;
NSMutableURLRequest *urlRequest = nil;
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of this, can we check:

if url == nil inside endpointURL and return it, otherwise do the same?

@jcavar
Copy link
Contributor

jcavar commented May 12, 2018

Great! Thank you @mendirattanishant.

@jcavar jcavar merged commit 45155cc into novastone-media:master May 12, 2018
@mendirattanishant
Copy link
Contributor Author

@jcavar should I update the readme?

@jcavar
Copy link
Contributor

jcavar commented May 14, 2018

Would be nice, yes.

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