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

Enhance Docker Configuration with Custom Data Path and other Var #171

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

MuhammadUsamaMX
Copy link
Contributor

Enhance Docker Configuration with Custom Data Path and other Var

Update Dockerfile for build
Updated the `docker-compose.yml` to introduce an environment variable `DATA_PATH`, defaulting to `./`. This allows users to easily customize the storage path for volumes while maintaining a simple local setup. Adjusted volume mappings to utilize the new variable for better flexibility and maintainability.
Fix Docker with Enhance Docker Configuration with Custom Data Path compose.yaml
Create .env
Update  compose.yaml with .env var
Update .env with other var
@mgrimace
Copy link
Contributor

mgrimace commented Oct 17, 2024

Interesting, and thanks for taking a look at Docker.

Please do take a look at my recent updates to the dockerfile (which reduced the number of RUN layers) #167 , and the added volume definitions in the sample compose #164.

re. implementing .env files:
Personal details like TZ, and a data path make sense for an .env. However, I think it might be better to implement that as a sample.env, and a line in the readme. Basically as 'optional' rather than default because it adds a layer of complexity. What do folks think?

However, using a .env for config options does not make sense to me. It just adds a layer of unnecessary abstraction IMO. To me, it makes more sense to edit the config.json directly and volume map it to the container (e.g., for values like headless, cron, etc., which are not private or identifying anyways). Env var overrides are meant to be a quick time saver to toggle on/off config options for testing, but using config.json is a much better persistent solution for docker users. I'd suggest reverting node-env, headless, cron, and run-on-start to defaults in the compose and not in an .env.

Next steps for .env

A .env would be useful if we could find a way to enter the account info directly into the compose file to prevent saving passwords in plaintext. I haven’t got there yet though!

Happy to chat this all through

@MuhammadUsamaMX
Copy link
Contributor Author

Thanks for the input! I agree that using a sample.env and adding it to the README makes sense to keep it optional and reduce complexity. For config options like headless, cron, and run-on-start, editing config.json directly and mapping it to the container seems like a better, more persistent approach. Using the .env for sensitive info, like credentials, would keep things clean. I also support exploring secure ways to handle account info without plaintext passwords. Let’s discuss further!

@mgrimace
Copy link
Contributor

Suggested draft sample compose, updated with above suggestions to include .env support.

Note for any .env values, I used :-default notation so that if a person doesn't know how to use .env, then useful default values are still entered. I'm not sure if this helps or just adds confusion. For example, if a person does not set APPDATA for the config, then it should default to ./config. What do you think?

Some things like 'headless' should not be changed for docker for any reason, so I kept it in as-is.
Consider changing the filename of .env to sample.env so it doesn't automatically become 'hidden' for users. I added quick instructions in the sample.env for saving it as a .env

Consider also keeping the data paths for the source script and config data separate (e.g., I put script source data in /compose/ and config data in /appdata/). I used 'appdata_path' for volumes below, if you want to specify a different source folder, that's fine too, I used 'source_path'.

Take a look and let me know what you think, and consider updating your PR with any suggestions below.

services:
  microsoft-rewards-script:
    build: .
    container_name: netsky
    environment:
      - TZ=${TZ:-America/Toronto} #change to your local timezone
      - NODE_ENV=production 
      - HEADLESS=true #do not change
      ### Customize your run schedule, default 5:00 am and 11:00 am, use crontab.guru for formatting
      - CRON_START_TIME=${CRON_START_TIME:-0 5,11 * * *}
      ### Run on start, set to false to only run the script per the cron schedule
      - RUN_ON_START=${RUN_ON_START:-true} 
      ### Optionally, set a different path to the script source files
      - SOURCE_PATH=${SOURCE_PATH:-./}  
    volumes:
      ### Replace '${APPDATA:-./}' with the path to your config files on your local machine, or use the sample.env
      - ${APPDATA_PATH:-./}/accounts.json:/usr/src/microsoft-rewards-script/dist/accounts.json
      - ${APPDATA_PATH:-./}/config.json:/usr/src/microsoft-rewards-script/dist/config.json 
      - ${APPDATA_PATH:-./}/sessions:/usr/src/microsoft-rewards-script/dist/browser/sessions #optional, saves your login session
    deploy:
      resources:
        limits:
          memory: 2048M    
    restart: unless-stopped

sample.env

### Save and rename this file as .env and place in the same folder as your compose.yaml
SOURCE_PATH=/home/ubuntu/source/netsky #customize the location of your script source data
APPDATA_PATH=/home/ubuntu/appdata/netsky #customize the location of config data
TZ=America/Toronto # Change to your local timezone
### Run on start, set to false to only run the script per the cron schedule
RUN_ON_START=true
### Customize your run schedule, default 5:00 am and 11:00 am, use crontab.guru for formatting
CRON_START_TIME=0 5,11 * * *

@mgrimace
Copy link
Contributor

TBH, I think it might be easier to just add a sample.env to the repo, and not change the compose. My thought is anyone who uses .env would likely know how to replace the values in the compose. I'm thinking it might just over-complicate the compose.

@mgrimace
Copy link
Contributor

Should we move this out of a PR and into an issue/feature request discussion for how to best implement this? I'd not recommend the changes as they are in the initial 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.

2 participants