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

Improvements before we create a PR to Shinobi's main repo #2

Open
1 of 4 tasks
viktorsmari opened this issue Mar 30, 2019 · 4 comments
Open
1 of 4 tasks

Improvements before we create a PR to Shinobi's main repo #2

viktorsmari opened this issue Mar 30, 2019 · 4 comments

Comments

@viktorsmari
Copy link
Contributor

viktorsmari commented Mar 30, 2019

Awesome work @acobaugh !

Just tried it and it works out of the box with 2 commands.

I noted down some ideas in order to make this even simpler to setup:

  • Make docker-compose.yml general, and add a docker-compose.prod.yml for customization (TLS, network setup, more containers etc)
  • Can we remove the dualstack network? It would remove an extra step in the setup.
  • Lock down mysql version, already did a PR for that Lock mysql version to 5.7 #1
  • Reduce number of files needed, so we can make a PR to the main repo, with ideally just the Dockerfile, docker-compose.yml and shinobi.env.example
  • Docker compose can use multiple files to extend the base file: https://docs.docker.com/compose/extends/
    • Example: docker-compose -f docker-compose.yml -f docker-compose.prod.yml up -d
  • The project cannot be started by simply doing docker-compose up because it requires you to add a dualstack network first. Do we need it? It can be extended in a docker-compose.prod.yml file f.x.
  • Could ENV vars possibly replace the super.sample.json and the docker-entrypoint.sh files?
@viktorsmari viktorsmari changed the title Improvements Improvements before we create a PR to the main repo Mar 30, 2019
@viktorsmari viktorsmari changed the title Improvements before we create a PR to the main repo Improvements before we create a PR to Shinobi's main repo Mar 30, 2019
@acobaugh
Copy link
Owner

The dualstack was something specific to my specific use case, where it needed to be on the same docker network as another compose stack running nginx/letsencrypt, on a dual stack v4/v6 network so I could reach it from outside my house on ipv6, and within the house on ipv4.

Give me the weekend to try separating out my configuration locally here.

We should definitely do something about super.json. ENV vars might work here, I forget how shinobi is using that. Maybe a simple sed in the entrypoint to replace some placeholders in super.json?

Open to suggestions.

@viktorsmari
Copy link
Contributor Author

viktorsmari commented Mar 30, 2019

Yes, I think at first we need to use the entrypoint file to use sed but I want to create an issue or a PR on the main Shinobi repo, and ask the maintainer to allow the Shinobi node application to use ENV vars if they are provided (else it will use the current default). Inspired by the 12 factor app.

Example mysql setup where SQLHOST=mysql is provided in the shinobi.env file

if (process.env.SQLHOST) {
  host = process.env.SQLHOST;   // Will become the 'mysql' container if using Docker
}else{
  host = '127.0.0.1'; // Default for people not using Docker
}
//EDIT: Or using a one-liner
host = process.env.SQLHOST || 'mysql'

SQLHOST could also be called DOCKER_SQLHOST or whatever makes sense.

That means when we supply these env vars with Docker, the app will use them.

This way we should not have to do some sed magic in an entrypoint file, and hopefully eliminate the need for it.

I am not sure which files need to support this in the main Shinobi application, but I found these in the entrypoint file. Are these the values we should provide ENV support for?

/opt/shinobi/conf.json

  • "key":"73ffd716-16ab-40f4-8c2e-aecbd3bc1d30"/"key":"'"${CRON_KEY}"'
  • "Motion":"d4b5feb4-8f9c-4b91-bfec-277c641fc5e3"/"Motion":"'"${PLUGINKEY_MOTION}"
  • "OpenCV":"644bb8aa-8066-44b6-955a-073e6a745c74"/"OpenCV":"'"${PLUGINKEY_OPENCV}"'
  • "OpenALPR":"9973e390-f6cd-44a4-86d7-954df863cea0"/"OpenALPR":"'"${PLUGINKEY_OPENALPR}"'

/opt/shinobi/super.json

  • "mail":"[email protected]"/"mail":"'"${ADMIN_USER}"'
  • "s/21232f297a57a5a743894a0e4a801fc3/${ADMIN_PASSWORD_MD5}

If we perfect this list, we can make a PR to the main repo just to accept ENV vars, then it should be easier for us, not having to use sed and again, eliminate the need for the entrypoint file.

@viktorsmari
Copy link
Contributor Author

FYI: I opened up a discussion about using ENV vars in the main repo:
https://gitlab.com/Shinobi-Systems/Shinobi/issues/52

@acobaugh
Copy link
Owner

acobaugh commented Apr 1, 2019

Yes, those are the values we'd want to source from ENV. Ideally we could abstract that so the ENV var name is predictable in some manner, based on JSON path, so we don't have tons of if{} statements, or have to modify the code every time a new variable is added.

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

No branches or pull requests

2 participants