Skip to content

Conversation

@SimonUnge
Copy link
Collaborator

Proposed Changes

With OTP 26, would be nice to config some amqp client settings easier, for example when using the federation plugin.

Types of Changes

What types of changes does your code introduce to this project?
Put an x in the boxes that apply

  • 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)
  • Build system and/or CI

Checklist

Put an x in the boxes that apply.
You can also fill these out after creating the PR.
If you're unsure about any of them, don't hesitate to ask on the mailing list.
We're here to help!
This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING.md document
  • I have signed the CA (see https://cla.pivotal.io/sign/rabbitmq)
  • I have added tests that prove my fix is effective or that my feature works
  • All tests pass locally with my changes
  • If relevant, I have added necessary documentation to https://github.com/rabbitmq/rabbitmq-website
  • If relevant, I have added this change to the first version(s) in release-notes that I expect to introduce it

Further Comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution
you did and what alternatives you considered, etc.

@mergify mergify bot added the bazel label Jun 7, 2024
@SimonUnge
Copy link
Collaborator Author

Note that the test case fails, as rabbit is not loading the config schema file for me when running the test (bazel). It does load it fine when I start up rabbit with bazel run ... though. I am clearly missing something... trying to figure it out...

@SimonUnge
Copy link
Collaborator Author

@pjk25 I am somehow messing something up, as the testcase is not picking up the schema file...

@SimonUnge SimonUnge marked this pull request as ready for review June 7, 2024 17:55
Copy link
Contributor

@gomoripeti gomoripeti left a comment

Choose a reason for hiding this comment

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

I wonder if it would make sense to add the same for amqp10_client for symmetry? (Or if there is any other way to not have to duplicate everything)
#11369

{mapping, "amqp_client.ssl_options.verify", "amqp_client.ssl_options.verify", [
{datatype, {enum, [verify_peer, verify_none]}}]}.

{mapping, "amqp_client.ssl_options.fail_if_no_peer_cert", "amqp_client.ssl_options.fail_if_no_peer_cert", [
Copy link
Contributor

Choose a reason for hiding this comment

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

I might be mistaken but isn't fail_if_no_peer_cert a server only option, and new OTP 26 ssl will complain if set for a client?
rabbitmq/rabbitmq-website#1933

Copy link
Collaborator

Choose a reason for hiding this comment

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

@gomoripeti you are correct, we should leave it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right I was a bit aggressive in my schema copy/pasting. I'll go through it and remove all server_options!

@michaelklishin
Copy link
Collaborator

@SimonUnge when you start a node, the rabbit app and all of its dependencies (or specifically the once you have asked for using RABBITMQ_ENABLED_PLUGINS) are on the code path.

When you run integration tests of this client, rabbit will not be on the list of dependencies.

I suspect some of our Bazel helpers do not consider that a project would not have rabbit as a dependency because in this repo, this is one of maybe two or three? subprojects where this is the case.

@michaelklishin
Copy link
Collaborator

This could agruably go into rabbit.schema. The schema won't be super useful outside of RabbitMQ, that is, when this library is used as a standalone library.

@SimonUnge
Copy link
Collaborator Author

@SimonUnge when you start a node, the rabbit app and all of its dependencies (or specifically the once you have asked for using RABBITMQ_ENABLED_PLUGINS) are on the code path.

When you run integration tests of this client, rabbit will not be on the list of dependencies.

I suspect some of our Bazel helpers do not consider that a project would not have rabbit as a dependency because in this repo, this is one of maybe two or three? subprojects where this is the case.

Aha good hint! I'll figure it out this weekend!

@SimonUnge
Copy link
Collaborator Author

This could agruably go into rabbit.schema. The schema won't be super useful outside of RabbitMQ, that is, when this library is used as a standalone library.

Ah ok yeah even simpler!

@SimonUnge
Copy link
Collaborator Author

Moved the schema into rabbit, and the tests.

As for what to do with amqp 1.0 - either duplicate the schema (or just use the same config?)

@SimonUnge SimonUnge force-pushed the amqp_client_schema branch from 90a53f3 to ca887bf Compare June 10, 2024 20:48
@michaelklishin
Copy link
Collaborator

AMQP 1.0 client can use amqp10_client. Or, more in line with what Message Containers use, amqp_client can be used for AMQP 1.0 and amqpl_client for AMQP 0-9-1 ("AMQP legacy", although the name can be questioned by some, the abbreviation is already used in RabbitMQ 3.13.0+ internally).

@SimonUnge
Copy link
Collaborator Author

AMQP 1.0 client can use amqp10_client. Or, more in line with what Message Containers use, amqp_client can be used for AMQP 1.0 and amqpl_client for AMQP 0-9-1 ("AMQP legacy", although the name can be questioned by some, the abbreviation is already used in RabbitMQ 3.13.0+ internally).

Ah ok, got it! And, naming aside, we do want to separate the two I take it? i.e give the option to configure amqp 0.9 client and 1.0 clients differently.

@michaelklishin
Copy link
Collaborator

The forced push was a rebase.

@SimonUnge
Copy link
Collaborator Author

I'll leave it as amqp_schema for now, and duplicate it for amqp10_client as I saw amqp10_client is used already (in one place, but still).

@michaelklishin
Copy link
Collaborator

The CI failures have to do with the (new) Bazel cache storage permission for external contributions.

I've tested this locally with

bazel test //deps/rabbit:config_schema_SUITE

@michaelklishin
Copy link
Collaborator

I had to revert this PR #11531.

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.

3 participants