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

Update docker docs #237

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Update docker docs #237

wants to merge 2 commits into from

Conversation

dmcken
Copy link

@dmcken dmcken commented Nov 12, 2021

  • Fixed docker-compose command documentation to use network_importer (not network-importer) as defined in docker-compose.yml
  • Added table of all currently defined environment variables to docs/configuration.md based on what I found in network_importer/config.py. I hope to work on this to fill in the missing entries as I would prefer to work with this project using docker.

Fixed docker-compose service name in example commands.
List all the currently defined environment variables. The empty values can be used as a focus on what to fix next so the service can be better dockerized.
Copy link
Contributor

@dgarros dgarros 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 for this PR, I added few comments and suggestions inline

| BATFISH_PORT_V2 | batfish | port_v2 |
| BATFISH_USE_SSL | batfish | use_ssl |
| | drivers.mapping | <should this be made into ENV variables?> |
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure how we could fit everything into an environment variable ... probably best to keep it as it for now

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
| | drivers.mapping | <should this be made into ENV variables?> |
| | drivers | mapping |

| | inventory | supported_platforms |
| | inventory | backend (duplicate of main.backend?) |
| | inventory.settings | <TODO> |
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
| | inventory.settings | <TODO> |
| | inventory.settings | Backend specific |

| | drivers.mapping | <should this be made into ENV variables?> |
| | inventory | supported_platforms |
| | inventory | backend (duplicate of main.backend?) |
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the doc is not correct, this setting doesn't exist.
please could you remove it from this table and update the doc as well ?

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