-
Notifications
You must be signed in to change notification settings - Fork 91
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
Support for dynamic port allocation #134
Conversation
3b87fb0
to
2d7e3fe
Compare
2d7e3fe
to
683f8ed
Compare
@@ -137,10 +136,6 @@ func (c Config) BinaryRepositoryURL(binaryRepositoryURL string) Config { | |||
return c | |||
} | |||
|
|||
func (c Config) GetConnectionURL() string { |
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.
We won't be able to remove this without bumping a major version, which I think we're unlikely to want to do for this change. Can you try and work it into here?
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.
I already pitched a solution attempt in our discussion in the issue within config.go and we both agreed that there should be no major logic there.
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.
Hey @mainbrain these changes wouldn't be backwards compatible meaning we'd need to do a major version bump, which I'd like to avoid. Can you have a go working the changes to be backwards compatible? Cheers
Hey @mainbrain I'll take a look again this weekend if you've made above requested changes. |
I pushed another variant. Maybe that's more to your liking. The trade off is: We are backwards compatible but the interface is less consistent, so I had to write more documentation. |
Support for dynamic port allocation
resolves #128