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 support to manipulate cluster name #104

Merged
merged 1 commit into from
Jul 21, 2017
Merged

Add support to manipulate cluster name #104

merged 1 commit into from
Jul 21, 2017

Conversation

sheepkiller
Copy link

Hi,

thank you for this great package, it's very useful for my project. Since it requires to programmaticaly rename the cluster, I added SetClusterName()/GetClusterName() functions.
Thanks.

regards,

Copy link
Owner

@michaelklishin michaelklishin left a comment

Choose a reason for hiding this comment

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

Thank you, the test needs a bit of work. I also think that we should consider making SetClusterName take a string instead of a struct.

@@ -265,6 +265,20 @@ var _ = Describe("Rabbithole", func() {
})
})

Context("PUT /cluster-name", func() {
It("Set cluster name", func() {
Copy link
Owner

Choose a reason for hiding this comment

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

This test does not restore cluster name to its original value.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, no problem. BTW, since cluster name is defined at run time and is hostname based, it may be more elegant and predictable to set arbitrarily a value in ./bin/ci/before_build.sh. Are you ok with that ?

Copy link
Owner

Choose a reason for hiding this comment

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

Yup. I don't think we depend on cluster name anywhere. rabbit@localhost is the value I'd use.

// => ClusterName, err

// Rename cluster
resp, err := rmqc.SetClusterName(ClusterName{Name: "rabbitmq@rabbit-hole"})
Copy link
Owner

Choose a reason for hiding this comment

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

I wonder if this should take a string… or reusing the same struct is actually OK. In most cases I've seen a cluster name is not set to a previously fetched value.

Copy link
Author

Choose a reason for hiding this comment

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

I initially chose to use a string as parameter, but I though passing a struct is more consistent with the API, since cluster name is set via the request body and not as part of path. I've have no problem to switch back to string.

Copy link
Owner

Choose a reason for hiding this comment

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

@gerhard and I discussed this and perhaps using a struct is not too odd. It would be more flexible should this part of the API change or require validation and it's not too weird for Go libraries to do something like this.

So no need for changing it. Thank you for responding so quickly!

  GetClusterName() to fetch cluster name
  SetClusterName() to rename the cluster
@sheepkiller
Copy link
Author

I fixed the tests and updated doc.go

@michaelklishin michaelklishin merged commit 978503b into michaelklishin:master Jul 21, 2017
@michaelklishin
Copy link
Owner

Thank you!

michaelklishin added a commit that referenced this pull request Sep 13, 2017
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.

2 participants