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

Dockerize + Discord Webhook #17

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

Conversation

probablypablito
Copy link

Added Docker support.

To do this I had to:

  1. Move config.json and auth.json to a separate directory (data).
  2. Remove forced UI interaction, and instead use the --account flag to access the account manager. The UI interaction made it impossible to automate, and I found this to be the easiest solution. Maybe adding a print statement to use the flag if you want to access account manager on each boot would be nice
  3. Publish my own Docker package on DockerHub. Feel free to publish your own image if you want to. I took the time to use buildx to have multi-arch support, so that shouldn't be an issue either :)

Discord Webhook Support

I only added this for the auth token expiring, as for password resets you get emails. It's quite annoying having the auth token expire and not knowing

Added a requirements.txt

This wasn't really necessary, but it is common courtesy for Python projects

- Added Docker support.
To do this I had to
      1) Move config.json and auth.json to a separate directory (data).
      2) Remove forced UI interaction, and instead use the  --account flag to access the account manager. The UI interaction made it impossible to automate, and I found this to be the easiest solution. Maybe adding a print statement to use the flag if you want to access account manager on each boot would be nice
   3) Publish my own Docker package on DockerHub. Feel free to publish your own image if you want to. I took the time to use buildx to have multi-arch support, so that shouldn't be an issue either :)

- Discord Webhook Support
I only added this for the auth token expiring, as for password resets you get emails. It's quite annoying having the auth token expire and not knowing

- Added a requirements.txt
This wasn't really necessary, but it is common courtesy for Python projects
@Salty-Coder
Copy link
Contributor

Full list of requirements already in PR #14

@Playeereq
Copy link
Contributor

webhook - #12
requirements.txt - #14

@Salty-Coder
Copy link
Contributor

Discord webhook options?
Full logging or only token expire logging could work nicely...

@probablypablito
Copy link
Author

probablypablito commented Aug 26, 2022

Requirements in #14 is inaccurate, it lists 3 extra dependencies all 3 of which are pre-installed with Python.

I also only saw your Discord Webhook PR after implementing mine, sorry
Yours seems better implemented than mine, with mine making use of ENV variables which while nice for Docker, not nice for standard use.

@Salty-Coder
Copy link
Contributor

Salty-Coder commented Aug 26, 2022

Requirements in #14 is inaccurate, it lists 3 extra dependencies all 3 of which are pre-installed with Python.

I also only saw your Discord Webhook PR after implementing mine, sorry

Ah. Didn't realise the 3 dependencies were core packages.

In regards to the webhooks: your implementation of it is very smart as my PR doesn't log auth errors. I think maybe an option in config of choosing full logging (#12) or choose token expiration logging (#17). That would give the user a nice choice.

@probablypablito
Copy link
Author

Error only vs full seems like a better solution to me. Auth errors were just the most important to me

@Salty-Coder
Copy link
Contributor

Yeah sounds good.
Either I'll try to sort that out and PR it or if you would like to then I'll leave it to you.
Either way, @PRO100KatYT hasn't merged any of my PRs or commented on them in over 5 months so I'm not sure if it will get merged at all...

@probablypablito
Copy link
Author

meh as long as we have a cool fork its all good.

personally i dont really want to work on this anymore as it works fine for my use case, but if you submit a PR ill add it to the docker image

@Salty-Coder
Copy link
Contributor

Yeah. If I get round to it I'll work on it but I'm not too bothered.
As my PRs don't get merged, I made my own fork a while ago with all my PR features.

@probablypablito
Copy link
Author

Awesome. Lmk when you commit it and I'll take the changes and add them the Docker image. Feel free to also steal my changes in my fork (Dockerfile, etc) and put them in yours. If you do that just mention me in the commit / add me as a contributor :)

@Salty-Coder
Copy link
Contributor

Thanks. I might yoink some of your code for my fork but I'll give you proper credit for it :)

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.

3 participants