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

Adding fluent builder withLink for addLink method #463

Closed

Conversation

orange-buffalo
Copy link
Contributor

It would be very convenient to add links to another containers using fluent builder. Currently addLink method has no fluent alternative. This PR introduces one: withLink.

@kiview
Copy link
Member

kiview commented Sep 26, 2017

AFAIK, linking is discouraged and using Docker networking should be preferred after all.

@orange-buffalo
Copy link
Contributor Author

Indeed, Docker recommends to avoid usage of legacy link functionality unless absolutely necessary.

Then I believe there are two logical alternatives: either addLink stays to be used with caution and withLink is introduced for fluent symmetry, or addLink method is deprecated. Does it make sense?

@kiview
Copy link
Member

kiview commented Sep 26, 2017

Networking in Testcontainers should be stable enough to deprecate addLink. What do you think @bsideup ?

@bsideup
Copy link
Member

bsideup commented Sep 27, 2017

@kiview sounds good :) Networks are stable, less error-prone and easier to use :)

@kiview
Copy link
Member

kiview commented Sep 27, 2017

@rnorth are you fine with it as well? Is addLink the only method using Docker links, or do we have to deprecate multiple methods?

@rnorth
Copy link
Member

rnorth commented Sep 27, 2017

I'm fine with deprecating use of links; this predates the existence of network support in Docker, and I'd agree that we've waited long enough.

There are a few things I'd like to do in the process of deprecating - I'll raise an issue for further tracking/discussion.

@orange-buffalo thank you for raising the PR, but would you be OK with closing based on the above?

@orange-buffalo
Copy link
Contributor Author

@rnorth, sure, I will close the PR.

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.

None yet

4 participants