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 a default value for the docker.network configuration #3471

Merged
merged 10 commits into from
Jun 13, 2018

Conversation

jbdoumenjou
Copy link
Member

@jbdoumenjou jbdoumenjou commented Jun 11, 2018

What does this PR do?

Add a default value for the traefik.docker.network configuration.

Motivation

Implement the #3449

More

  • Added/updated tests
  • Added/updated documentation

Additional Notes

I can't test it due to the limitation of the docker test library (we can't manipulate the networks).

Copy link
Member

@emilevauge emilevauge left a comment

Choose a reason for hiding this comment

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

Few comments in the code.
Isn't it possible to add integration tests?

@@ -71,6 +71,12 @@ usebindportip = true
#
swarmMode = false

# Define a default docker network to use for connections to all containers.
Copy link
Member

@emilevauge emilevauge Jun 11, 2018

Choose a reason for hiding this comment

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

Maybe we should add the fact that it's possible to override this with the label traefik.docker.network

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -45,6 +45,7 @@ type Provider struct {
ExposedByDefault bool `description:"Expose containers by default" export:"true"`
UseBindPortIP bool `description:"Use the ip address from the bound port, rather than from the inner network" export:"true"`
SwarmMode bool `description:"Use Docker on Swarm Mode" export:"true"`
Network string `description:"Default Docker network used" export:"true"`
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we call it DefaultNetwork instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

To be homogeneous with domain, we need to keep Network.

@ldez ldez added the kind/enhancement a new or improved feature. label Jun 11, 2018
@ldez ldez changed the title Feature - Add a default value for the docker.network configuration Add a default value for the docker.network configuration Jun 11, 2018
@@ -45,6 +45,7 @@ type Provider struct {
ExposedByDefault bool `description:"Expose containers by default" export:"true"`
UseBindPortIP bool `description:"Use the ip address from the bound port, rather than from the inner network" export:"true"`
SwarmMode bool `description:"Use Docker on Swarm Mode" export:"true"`
DefaultNetwork string `description:"Default Docker network used" export:"true"`
Copy link
Contributor

Choose a reason for hiding this comment

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

To be homogeneous with domain, we need to keep Network.

Copy link
Member

Choose a reason for hiding this comment

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

😁

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you both ok with Network ? :)

Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

Could you please fix the make validate

docs/configuration/backends/docker.md:75:9: "overriden" is a misspelling of "overridden"
docs/configuration/backends/docker.md:136:9: "overriden" is a misspelling of "overridden"

@jbdoumenjou jbdoumenjou force-pushed the feature/default-docker-network branch from 4d1ac5f to d209e36 Compare June 11, 2018 12:48
Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

@jbdoumenjou jbdoumenjou force-pushed the feature/default-docker-network branch from d434cb9 to 50f47ae Compare June 12, 2018 15:38
Copy link
Member

@mmatur mmatur left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ldez ldez left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏 🎉

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

Successfully merging this pull request may close these issues.

6 participants