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

refs #309, unmarshalling AutoDelete: "undefined" to false not true #312

Merged

Conversation

pafmaf
Copy link
Contributor

@pafmaf pafmaf commented May 28, 2024

exchange.auto_delete should not be undefined -> now defaults to false when it's string "undefined", not true.

  • by default auto_delete is false
  • when undefined, it's more likely to behave like false

Please see the details / origin of this change in #309 .
I could not find any explanation for the changes in #207 .

Why?

Sometimes there are issues with deploying exchanges using topology-operator. Unfortunately, it's a bit hard to reproduce, so I did not find the real problem yet. The exchanges are unchanged, and all set to auto_delete: false when being created, but the diff still says that auto_delete changed:


This is a draft, and I'm not sure how to test this, since I can only un-marshall the JSON, but setting up an exchange with undefined would require to "exploit a bug" in rabbitmq. (You shouldn't be able to create an exchange with auto_delete: undefined in the first place.)


If this change leads to problems for lib users, they should fix the original problem -> it may lead to fixes in rabbit? idk.

…false` not `true`

`exchange.auto_delete` should not be `undefined` -> now defaults to
`false` when it's string `"undefined"`.
@pafmaf
Copy link
Contributor Author

pafmaf commented Jun 10, 2024

https://github.com/michaelklishin/rabbit-hole/actions/runs/9272433603/job/25753640675?pr=312#step:8:26

I can make the same test fail locally when running tests against the rabbitmq:3.13-management docker image (on main). So I don't think this is an issue with this PR.
It's still fine with 3.12 .

Not sure why it didn't break before though. - But maybe the 3.13 docker tag was overwritten somewhere in between and the newer versions cause problems.

@michaelklishin michaelklishin merged commit 8b4162c into michaelklishin:main Jun 15, 2024
0 of 2 checks 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