Skip to content
This repository has been archived by the owner on Nov 14, 2020. It is now read-only.

Feature/add shovel resource #48

Merged
merged 1 commit into from
Jul 17, 2020
Merged

Feature/add shovel resource #48

merged 1 commit into from
Jul 17, 2020

Conversation

justinbaur
Copy link
Contributor

Submitting to add support for the rabbitmq shovel as the underling rabbit hole lib supports it already.

@justinbaur
Copy link
Contributor Author

justinbaur commented Jan 20, 2020

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
sudo rabbitmq-plugins enable rabbitmq_shovel but not sure if that apply's to the CI.

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.

@justinbaur
Copy link
Contributor Author

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.

Copy link
Contributor

@cyrilgdn cyrilgdn left a 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

rabbitmq/resource_shovel.go Outdated Show resolved Hide resolved
rabbitmq/resource_shovel.go Outdated Show resolved Hide resolved
rabbitmq/resource_shovel_test.go Show resolved Hide resolved
website/docs/r/shovel.html.markdown Outdated Show resolved Hide resolved
@justinbaur
Copy link
Contributor Author

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=
Copy link
Contributor

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 🤔

Copy link
Contributor Author

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?

niclic added a commit to niclic/terraform-provider-rabbitmq that referenced this pull request May 1, 2020
niclic added a commit to niclic/terraform-provider-rabbitmq that referenced this pull request Jun 17, 2020
@akurz
Copy link

akurz commented Jun 22, 2020

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,
Andreas

@justinbaur
Copy link
Contributor Author

@akurz I'll be able to fix the conflicts but it still has a change requested review. @cyrilgdn does anything else need to be changed in this PR?

@cyrilgdn
Copy link
Contributor

does anything else need to be changed in this PR?

@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.

Is there anything I could help to make this happen @cyrilgdn

@akurz You can test the PR with your use case if you know how to do it. It would be very helpful.

@akurz
Copy link

akurz commented Jul 2, 2020

does anything else need to be changed in this PR?

@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.

Is there anything I could help to make this happen @cyrilgdn

@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

Copy link
Contributor

@cyrilgdn cyrilgdn left a 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

@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
</li>
</li>

cyrilgdn pushed a commit that referenced this pull request Jul 3, 2020
* 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.
@akurz
Copy link

akurz commented Jul 8, 2020

@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 :-)

@ghost ghost removed the waiting-response label Jul 8, 2020
Copy link
Contributor

@cyrilgdn cyrilgdn left a 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.

@cyrilgdn cyrilgdn merged commit fdcca45 into hashicorp:master Jul 17, 2020
@akurz
Copy link

akurz commented Jul 17, 2020

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.

@cyrilgdn
Copy link
Contributor

@akurz Actually v1.4.0 has just been released with the shovel resource 😅

But feel free to open a PR, we can easily trigger new releases.

@justinbaur
Copy link
Contributor Author

@cyrilgdn Perfect! Thanks for doing that.

@akurz Thanks for testing as well.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants