-
Notifications
You must be signed in to change notification settings - Fork 79
Feature/add shovel resource #48
Feature/add shovel resource #48
Conversation
I believe the reason for failure is due in part to the shovel plugin not being enabled when the CI engine runs the ACC tests. I can update the deploy.sh with the following I was able to validate locally with rabbitmq running in docker without the plugin enabled and receiving the same error when make testacc was ran. |
My mistake, realized now I should have been testing against the same steps locally that travis ci performs. I used a separate rabbitmq instance for testing. I will be testing again locally and update the scripts folder for required changes to get the shovel plugin enabled in the docker image used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @justinbaur ,
Thanks a lot for your work 👍
All my comments are linked to same point, you just need to identify a resource with shovelName@vhost as we can create 2 shovels with the same name in 2 different vhosts.
To enable the plugin for your tests and for Travis, you can simply create a file in scripts
directory containing the list of plugins we want to enable:
[rabbitmq_management,rabbitmq_shovel_management].
(the trailing point is important 😄 )
And mount it in the container thanks to the docker-compose.yml
file:
$ git diff docker-compose.yml
diff --git scripts/docker-compose.yml scripts/docker-compose.yml
index 7504a17..c6d5f28 100644
--- scripts/docker-compose.yml
+++ scripts/docker-compose.yml
@@ -6,5 +6,7 @@ services:
environment:
RABBITMQ_DEFAULT_USER: ${RABBITMQ_USERNAME:-guest}
RABBITMQ_DEFAULT_PASS: ${RABBITMQ_PASSWORD:-guest}
+ volumes:
+ - ./enabled_plugins:/etc/rabbitmq/enabled_plugins
ports:
- 15672:15672
Not sure why Rabbit 3.6 keeps failing on declaring the queue and exchange in travisci. I've ran this against the docker-compose with 3.6 on Linux and was able to pass. |
go.sum
Outdated
@@ -290,9 +290,11 @@ github.com/oklog/run v1.0.0/go.mod h1:dlhp/R75TPv97u0XWUtDeV/lRKWPKSdTuV0TZvrmrQ | |||
github.com/onsi/ginkgo v1.6.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= | |||
github.com/onsi/ginkgo v1.7.0 h1:WSHQ+IS43OoUrWtD1/bbclrwK8TTH5hzp+umCiuxHgs= | |||
github.com/onsi/ginkgo v1.7.0/go.mod h1:lLunBs/Ym6LB5Z9jYTR76FiuTmxDTDusOGeTQH+WWjE= | |||
github.com/onsi/ginkgo v1.8.0 h1:VkHVNpR4iVnU8XQR6DBm8BqYjN7CRzw+xKUbVVbbW9w= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you know why does this file change? You didn't change any dependencies so it should not 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file would have changed when I merged the changes from "Switch to rabit-hole 2.0.0" commit 5470b5c
Would it make more sense to close the PR and rebase on my master and do a new PR to this master?
Hi all, I'd really be interested in getting this feature pull request in. Is there anything I could help to make this happen @cyrilgdn, @justinbaur? Cheers, |
@justinbaur We just need me to make the review 🤦 I have dedicated time for providers next Friday and I'll start with that if I didn't get the time before.
@akurz You can test the PR with your use case if you know how to do it. It would be very helpful. |
Sure, I can give it a try @cyrilgdn and will let you know if all works fine for me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
Just need to rebase on master
, rabbithole has been upgraded to v2 (so you'll have to modify the import
).
You should not have modifications in go.sum
anymore after the rebase
website/rabbitmq.erb
Outdated
@@ -37,6 +37,9 @@ | |||
<li<%= sidebar_current("docs-rabbitmq-resource-vhost") %>> | |||
<a href="/docs/providers/rabbitmq/r/vhost.html">rabbitmq_vhost</a> | |||
</li> | |||
<li<%= sidebar_current("docs-rabbitmq-resource-shovel") %>> | |||
<a href="/docs/providers/rabbitmq/r/shovel.html">rabbitmq_shovel</a> | |||
</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
</li> | |
</li> |
* Add Federation Upstream resource. * Add Component attribute to Federation Upstream. -- attribute is computed (i.e. read only) * Parse Federation Upstream id. * Import Federation Upstream. * Upgrade module replace to use remote branch. -- this is temporary. * Create a generic parseId function - move to util.go and refactor code to use this. * Move parseId functions to util. * Create federation-upstream.html.markdown -- first draft -- update with discussion of default values * Defaults for some federation attributes. * Include default values in Federation docs. * Remove replace from go.mod * Don't cache tests by default. * Ensure federation plugins are installed during build. -- after #48 * Tests should assert resource attributes. * Refactor tests. * Add a more complete federation example.
@cyrilgdn,@justinbaur I have successfully tested this pull request - rebased on current master. I was able to create a working shovel setup in one of our test clusters. Unfortunately rabbithole does not seem to support shovel connections with AMQP1.0 links as needed for Azure Eventhub - but that's another story :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@akurz Thanks for your help 👍
@justinbaur I allowed myself to rebase & squash this PR for you so we can merge it and include it in the coming release.
You're welcome @cyrilgdn! Do you have a rough time line for the next release? I'd also like to send a pull-request that adds the missing Shovel parameters to the resource. |
Submitting to add support for the rabbitmq shovel as the underling rabbit hole lib supports it already.