search

MeowWolf / node-red-contrib-amqp

AMQP nodes for node-red
22 stars 18 forks source link

Check exchange & queue instead of assert #49

Open David-Lor opened 2 years ago

David-Lor commented 2 years ago

@MeowWolf @amodelbello I was having the same problems as in #24 (wanted to consume a queue that has custom arguments, which is currently not supported by this node). Then I noticed that for consumption, the assertExchange/assertQueue methods are used, which create the exchange or queue that is being used - or fail if it already exists but the configuration mismatch.

So I tried replacing those calls with checkExchange/checkQueue instead, and now I can consume from special queues, like queues with TTL or of type Quorum. Maybe not the ideal solution (passing the arguments would be better but a bit more complex), but I think this is a nice feature.

If you want to get this merged, I should include a configuration checkbox on the nodes, so this behaviour can be toggled.

Other considerations:

David-Lor commented 2 years ago

Would you be interested on getting this merged? In which case I'd advance on the TODOs

penguineer commented 2 years ago

I ended up here because I have the same problem: Queues with custom parameters (ttl) and exceptions on node-red because they do not match the generic configuration.

This PR does exactly what I need. How can I help?

amodelbello commented 2 years ago

Sorry this dropped off my radar. It does seem like a good addition. I may have some time next week (probably towards the end) to assess/test and, if all goes well, merge.

penguineer commented 2 years ago

I don't have much experience with the node amqp library. With pika for python I tried to implement a logic where I where if a queue exists and create it if it doesn't. However, the check is destructive.

If the libamqp can do this check without killing the connection, this could also be a guard around creating the connection.

As a result the node would create missing resources, but not stumble over missing parameters.

amodelbello commented 2 years ago

@penguineer & @David-Lor I've pushed up a change to the PR that adds new boolean fields to the exchange and queue configs allowing the toggling between assert and check. Right now the values are hard-coded and set to assert. I couldn't merge the PR as is because it would break (or change rather) current functionality, i.e. the behavior when an exchange or queue does not already exist. By defaulting to assert we make sure that existing functionality remains the same.

@David-Lor - Please proceed with your proposal to add front-end config for this value, that is if you're still interested after five months of inactivity on my part. Sorry for the wait. I think this will be a nice addition though.

One last thing - the PR originally broke the unit tests because they were expecting the assert* methods to be called but the new code only called check*. Make sure you run the tests before pushing new changes up. Also the coverage thresholds are no longer met with the new methods added. Please make sure to add new tests for the new functionality you write. If you're not comfortable doing this I can probably write them after you push up a working change.

David-Lor commented 2 years ago

Hi @amodelbello thank you for looking into it :)

I'll try to take a look this weekend.

penguineer commented 2 years ago

Thank you both!

I am still thinking about the label for the checkbox:

However, this is more philosophical and both solutions will help solve the problem.

amodelbello commented 2 years ago

@penguineer - I'd prefer the latter. We should opt to keep existing functionality as-is as much as possible and making it the default does this.

Perhaps "Do not assert (check only)" for each checkbox?

And if you're up for it I think a really nice addition would be to add to the documentation section in the right gutter. You could explain in more detail what the option means and how it allows you so specify custom message options if used.

David-Lor commented 2 years ago

I guess having the current behaviour by default (the assert) would be preferable, to avoid breaking changes

penguineer commented 2 years ago

I'm sorry I held up this PR!

I am not too skilled in JavaScript and node.js, nor do I have a working environment to develop and test plugins for node-red. My suggestions were merely coming from a perspective of interface design and thinking about the "API".

@David-Lor I understood that you had almost solved this?

David-Lor commented 2 years ago

Sorry, on my side I was unable to continue with what was pending, and right now I can't ensure when I can look into it.

David-Lor commented 2 years ago

@amodelbello I was trying to advance on this, but I'm having some trouble local-testing. Do you know how to install the local development node on NodeRed, for trying it out? I am doing the following:

But when I restart nodered, the node is not loaded and logs this error:

23 Jul 09:03:15 - [warn] Failed to enable node:
23 Jul 09:03:15 - [warn]  - amqp-in-manual-ack : Error: Cannot find module '/usr/src/node-red/node_modules/@meowwolf/node-red-contrib-amqp/build/src/nodes/amqp-in-manual-ack.js'
Require stack:
- /usr/src/node-red/node_modules/@node-red/registry/lib/loader.js
- /usr/src/node-red/node_modules/@node-red/registry/lib/index.js
- /usr/src/node-red/node_modules/@node-red/runtime/lib/nodes/index.js
- /usr/src/node-red/node_modules/@node-red/runtime/lib/index.js
- /usr/src/node-red/node_modules/node-red/lib/red.js
- /usr/src/node-red/node_modules/node-red/red.js
amodelbello commented 2 years ago

@David-Lor, For local development I install the node from the local file system. Your package.json in your local node-red instance should have a line like this:

"@meowwolf/node-red-contrib-amqp": "file:/[absolute path to]/node-red-contrib-amqp",

In the amqp project on your file system run npm start which uses nodemon to rebuild (transpile typescript to javascript) after every change. Each time the node is rebuilt, you will then have to restart the node-red instance to pick up the changes in the node.

Does this help?

David-Lor commented 2 years ago

@amodelbello Yeah, that worked (:

penguineer commented 2 years ago

(Was https://github.com/MeowWolf/node-red-contrib-amqp/pull/49/commits/4931ba33333dea97906a051e3867ae2a37e7d86c on purpose? IMHO this should have been a rebase?)

Thank you for bringing the PR forward! I am still very interested in the result, but, too, did not manage to set up a working development environment. I am happily available for feedback and discussions tho.,

David-Lor commented 2 years ago

(Was 4931ba3 on purpose? IMHO this should have been a rebase?)

Yes, I just resolved a conflict from Github interface. I intend to perform a squash merge though, so I think it doesn't matter much.

th0th commented 1 year ago

Is there anyway I can help with this one? Getting this merged will help me a lot.

David-Lor commented 1 year ago

@th0th Sadly I cannot complete it. The only thing remaining, I think it should be just adding the checkboxes to the frontend for enabling/disabling the assert. If somebody wants to continue with it, can take the code from my branch/PR and upload it.

penguineer commented 1 year ago

Sadly I cannot complete it.

Oh :(

The only thing remaining, I think it should be just adding the checkboxes to the frontend for enabling/disabling the assert. If somebody wants to continue with it, can take the code from my branch/PR and upload it.

I propose this PR stays open until there is a replacement? Should be relatively easy to cherry-pick your work and continue from there (from the git perspective).

Is there a good tutorial/documentation on how to set up a workbench for NodeRed node development? (For me, it is not hard to write the code, but it seems to be a lot of effort to set up a test environment.)

David-Lor commented 1 year ago

@penguineer For local development you can install nodered in local or a Docker. I've asked @amodelbello back in time how to use the WIP extension for trying it while working on it: https://github.com/MeowWolf/node-red-contrib-amqp/pull/49#issuecomment-1193142884