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

Add new parameter autoconvert for rabbitmq_parameter #865

Merged
merged 1 commit into from
Nov 27, 2020

Conversation

joec4i
Copy link
Contributor

@joec4i joec4i commented Nov 25, 2020

Pull Request (PR) description

Add a new parameter autoconvert for rabbitmq_parameter to allow users to disable auto type conversion from numeric strings to integers if needed.

This Pull Request (PR) fixes the following issues

We have several x-content-hash exchanges which use a numeric string as the routing key. Due to the munge_value implementation, the routing key would be converted to an integer.

As a result, a valid payload to create a shovel as below

{
  "component": "shovel",
  "vhost": "foo",
  "name": "test",
  "value": {
    "src-uri": "amqps://user:[redacted]@rabbit.service.consul:5671/foo",
    "src-exchange-key": "5",
    "dest-uri": "amqps://user:[redacted]@rabbit.service.consul:5671/bar",
    "dest-exchange-key": "5",
    "ack-mode": "on-confirm",
    "src-protocol": "amqp091",
    "dest-protocol": "amqp091",
    "src-exchange": "indexer_document_requests",
    "src-delete-after": "never",
    "dest-exchange": "indexer_document_requests",
    "dest-add-forward-headers": false
  }
}

will be converted to

{
  "component": "shovel",
  "vhost": "foo",
  "name": "test",
  "value": {
    "src-uri": "amqps://user:[redacted]@rabbit.service.consul:5671/foo",
    "src-exchange-key": 5,
    "dest-uri": "amqps://user:[redacted]@rabbit.service.consul:5671/bar",
    "dest-exchange-key": 5,
    "ack-mode": "on-confirm",
    "src-protocol": "amqp091",
    "dest-protocol": "amqp091",
    "src-exchange": "indexer_document_requests",
    "src-delete-after": "never",
    "dest-exchange": "indexer_document_requests",
    "dest-add-forward-headers": false
  }
}

Because RabbitMQ doesn't accept exchange keys to be integers, it'll throw out validation errors:

Error:
Validation failed
dest-exchange-key should be binary, actually was 5
src-exchange-key should be binary, actually was 5

This PR would allow the auto type conversion to be disabled and therefore numeric strings to be included in the RabbitMQ parameter payload if needed. The users would be responsible to ensure the right date types are used if autoconvert is set to false.

Copy link
Contributor

@wyardley wyardley left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me, provided it’s used carefully. Would it be good to make the parameter refer to autoconvert vs munge?

@joec4i joec4i changed the title Add new parameter skip_munging for rabbitmq_parameter Add new parameter autoconvert for rabbitmq_parameter Nov 26, 2020
@joec4i
Copy link
Contributor Author

joec4i commented Nov 26, 2020

Looks reasonable to me, provided it’s used carefully. Would it be good to make the parameter refer to autoconvert vs munge?

Good point @wyardley . I've renamed the parameter to autoconvert which defaults to true. Please have another look. Thanks!

Copy link
Contributor

@wyardley wyardley left a comment

Choose a reason for hiding this comment

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

LGTM. Would you mind squashing down to one commit in your branch?

@joec4i
Copy link
Contributor Author

joec4i commented Nov 27, 2020

Thanks, Will! It's in one commit now.

@wyardley
Copy link
Contributor

Cool. Going to leave this open for a day or two in case anyone else wants to review / comment. I’ll try to cut a new release after merging this.

Copy link
Member

@nmaludy nmaludy left a comment

Choose a reason for hiding this comment

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

LGTM

@wyardley wyardley merged commit 5c28cc6 into voxpupuli:master Nov 27, 2020
@wyardley wyardley mentioned this pull request Dec 1, 2020
wyardley added a commit to wyardley/puppet-rabbitmq that referenced this pull request Dec 1, 2020
release 10.3.0:

* Add new parameter autoconvert for rabbitmq_parameter (@joec4i) (voxpupuli#865)
* Ensure :autoconvert is initialized before :value for rabbitmq_parameter (@joec4i) (voxpupuli#867)
* modulesync 3.1.0 (@bastelfreak) (voxpupuli#864)
wyardley added a commit to wyardley/puppet-rabbitmq that referenced this pull request Dec 1, 2020
release 10.3.0:

* Add new parameter autoconvert for rabbitmq_parameter (@joec4i) (voxpupuli#865)
* Ensure :autoconvert is initialized before :value for rabbitmq_parameter
  (@joec4i) (voxpupuli#867)
* modulesync 3.1.0 (@bastelfreak) (voxpupuli#864)
* modulesync 4.0.0 (@bastelfreak) (voxpupuli#866)
wyardley added a commit to wyardley/puppet-rabbitmq that referenced this pull request Dec 1, 2020
* Add new parameter autoconvert for rabbitmq_parameter (@joec4i) (voxpupuli#865)
* Ensure :autoconvert is initialized before :value for rabbitmq_parameter
  (@joec4i) (voxpupuli#867)
* modulesync 3.1.0 (@bastelfreak) (voxpupuli#864)
* modulesync 4.0.0 (@bastelfreak) (voxpupuli#866)
cegeka-jenkins pushed a commit to cegeka/puppet-rabbitmq that referenced this pull request Mar 26, 2021
* Add new parameter autoconvert for rabbitmq_parameter (@joec4i) (voxpupuli#865)
* Ensure :autoconvert is initialized before :value for rabbitmq_parameter
  (@joec4i) (voxpupuli#867)
* modulesync 3.1.0 (@bastelfreak) (voxpupuli#864)
* modulesync 4.0.0 (@bastelfreak) (voxpupuli#866)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants