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

Access token connection #230

Merged
merged 7 commits into from Mar 5, 2024

Conversation

aurban-iqmessenger
Copy link
Contributor

Proposed Changes

This pull request aims to add an update-secret protocol extension to the client. It's already possible to connect to the server with an access token using a regular connect method but there is no possibility to update this token as it expires and this method achieves that. This feature is already present in the Java client.

Additionally, I've updated the Dockerfile to point to the current RabbitMQ image as provided in Installation guides. It's impossible to use an old Dockerfile because it points to the long time dead Bintray. Apt Bintray-related files are also removed.

On top of that, it's impossible to run the project on Apple Silicon arch. I've made necessary changes but only locally and haven't included those in this pr so as not to break Intel arch compatibility.

I haven't checked the last box in the checklist below. Both CocoaAsyncSocket and JKVValue as they are right now have too low minimal target for the current XCode. But it doesn't seem to me I can do a pr for them since there is no real activity in those projects, a lot of stale prs, including the ones trying to up the deployment target, and this situation is like that for years already. I guess people are just including them with CocoaPods and up the target using post installation script feature of pods. In order to run tests for this project I checked those two libs locally, upped their targets and pointed Carthage to these local projects.

Types of Changes

What types of changes does your code introduce to this project?

  • Bug fix (non-breaking change which fixes issue #NNNN)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause an observable behavior change in existing systems)
  • Documentation improvements (corrections, new content, etc)
  • Cosmetic change (whitespace, formatting, etc)

Checklist

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • All tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in related repositories

Copy link
Member

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

Thank you for taking the time to contribute this protocol extension.

As far as I can tell, all these changes are handcrafted. IIRC this client does use a codegen (under ./codegen), so we should either remove the codegen or add this extension to the source XML file and re-generate.

I don't feel strongly either way.

What I do feel strongly about is PRs that mess with license headers in addition to other changes, and put in incorrect/outdated information on top of that. I'm afraid I cannot accept a PR with license changes that stick MPL 1.1 everywhere.

RabbitMQ has switched to MPL 2.0 severao years ago.

@@ -1,5 +1,5 @@
// This source code is dual-licensed under the Mozilla Public License ("MPL"),
// version 2.0 and the Apache License ("ASL"), version 2.0.
// version 1.1 and the Apache License ("ASL"), version 2.0.
Copy link
Member

Choose a reason for hiding this comment

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

RabbitMQ has changed its license to MPL 2.0 years ago. If this is not reflected somewhere in this client, it should be updated but in a separate PR. Please back out all license header changes.

@@ -1,4 +1,44 @@
// License goes here
// This source code is dual-licensed under the Mozilla Public License ("MPL"),
// version 1.1 and the Apache License ("ASL"), version 2.0.
Copy link
Member

Choose a reason for hiding this comment

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

Same as above, the license of open source RabbitMQ and its clients is now MPL 2.0 (plus usually at least one other license for client libraries).

@aurban-iqmessenger
Copy link
Contributor Author

No, you're wrong about the changes being handcrafted, I've used the codegen and properly added changes to XML for it to generate needed classes. That's actually exactly why all of the licenses are changed -- as soon as I ran the codegen it changed all of the headers; it's not like I wished to mess with licenses myself. But now I see there actually was a way to run codegen without changing licenses. Just have rolled the licenses back.

@michaelklishin
Copy link
Member

OK, thanks for confirming, indeed this client's code generator pre-dates the switch to MPL 2.

@michaelklishin michaelklishin merged commit 30538c8 into rabbitmq:main Mar 5, 2024
1 check passed
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.

None yet

2 participants