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

Make it possible for apps to provide a custom connection name #156

Merged
merged 2 commits into from
Mar 12, 2019

Conversation

michaelklishin
Copy link
Member

Closes #155, references #154.

@grsteffens it took a couple of attempts, I like this API better than what I had in mind at first (a simpler way to instantiate RMQConnectionConfig which is not really meant to be used by 99% of users).

Copy link

@grsteffens grsteffens left a comment

Choose a reason for hiding this comment

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

Looks like this will work, thanks again for getting this done so quickly. Assuming this has to be approved by another collaborator.

@michaelklishin
Copy link
Member Author

michaelklishin commented Mar 12, 2019

It does work (you can see user-provided connection name in server logs and Wireshark), the question is whether

let conn = RMQConnection(
  uri: IntegrationHelper.defaultEndpoint,
  userProvidedConnectionName: "testUserProvidedConnectionName.1",
  delegate: RMQConnectionDelegateLogger())
conn.start()

is an API we want to expose. Compared to my first attempt it is definitely more straightforward and not too different from other clients. So let's keep it. More selectors that include userProvidedConnectionName can be added as needed.

Thanks for the review.

@grsteffens
Copy link

Agreed. It also syncs up pretty well with how the Java client works (where the name is passed into the factory), which is nice for parity's sake. My apologies before when I said "looks like this will work", I saw that it was functionally working, I just meant work from an architectural standpoint which it does as well.

Please let me know if there is anything I can contribute to in this library as I like the collaboration. Maybe I could become a collaborator. Thanks again, Michael!

@michaelklishin michaelklishin merged commit 8c0d383 into master Mar 12, 2019
@michaelklishin michaelklishin deleted the rabbitmq-objc-client-155 branch March 12, 2019 15:30
@michaelklishin
Copy link
Member Author

@grsteffens I'm happy to add you as a collaborator. We have a number of issues still open and I will file a couple of things I've noticed while working on this client in the last few months. I'd like to begin addressing things such as #24 and #31 after we produce a new release (#148) but there's an annoying issue with a couple of test cases hanging and failing for a reason I could not so far track down. I'll file a new issue about that now.

@michaelklishin
Copy link
Member Author

@grsteffens check collaborator invitations, you should have one ;) In case you plan to look into an issue, just leave a comment.

I'd suggest setting up the test environment with Docker (see ./docker) and then manually with x509 certificate authentication enabled on the RabbitMQ node would be a good first step. CONTRIBUTING.md should be up-to-date.

@grsteffens
Copy link

Awesome, thanks @michaelklishin. I will work on setting up my test environment and then keep my eye out for new issues that you open. After I set up my environment, I can also offer a second set of eyes on those failing and hanging test cases to help expedite the release.

@michaelklishin
Copy link
Member Author

The release doesn't have to wait for #157. This isn't something new and FWIW I don't recall any similar reports from actual users.

@grsteffens
Copy link

Ah, understood

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