luigi311 / JellyPlex-Watched

Sync watched between jellyfin and plex locally
GNU General Public License v3.0
394 stars 22 forks source link

General build improvements #66

Closed Nicba1010 closed 1 year ago

Nicba1010 commented 1 year ago

Hi,

I've been looking at using this container but it seemed unnecessarily large. First off I looked at the part where you install build-essentials which still confuses me and I'd appreciate if you can tell me why you did that, I don't want to break something. Secondly, I added a python:3-alpine variant, which consumes even less space. I tested it on my local Plex and Jellyfin installation (Plex->Jellyfin dry run) and everything seems to work.

To sum up the additions in these PR:

I'd love if you could give me some feedback :)

Kindest regards, Roberto@Octris

Nicba1010 commented 1 year ago

Sorry about the force-push, had to sign my commits.

luigi311 commented 1 year ago

Most of this looks perfect to me!

I believe the build essential was from a long time ago and I haven't touched the dockerfile in a long time so lots of things have changed since then so we don't need it anymore.

We should still have a latest tag so we do not break existing deployments. We can do the following tag system which I think I've seen around latest <- points to alpine since it should be smaller latest-slim <- points to slim latest-alpine <- points to alpine

Pinning versions should be fine now since things are working and stable.

Can you change the target to dev branch

luigi311 commented 1 year ago

Looks like you merged in the refactor black.whitelist processing on this one

Nicba1010 commented 1 year ago

Oh crap, I'm sorry

Nicba1010 commented 1 year ago

@luigi311 removed that commit

luigi311 commented 1 year ago

No worries.

Looks like the dockerhub login fails, we might need to add in a check for that docker username like we had on the metadata before

luigi311 commented 1 year ago

should be as simple as replacing the if condition on the dockerhub login to

env: DOCKER_USERNAME: ${{ secrets.DOCKER_USERNAME }} if: "${{ env.DOCKER_USERNAME != '' }}"

Nicba1010 commented 1 year ago

Done, I think. Any reason you use it via the env variable and not directly from secrets?

luigi311 commented 1 year ago

You arent able to use secrets in if condition checks for steps for whatever reason https://github.com/orgs/community/discussions/26726

Nicba1010 commented 1 year ago

Didn't know that, thanks haha

luigi311 commented 1 year ago

looks like its failing to push to ghcr.io for some reason. The tags and everything look correct so i dont see why it would be failing

Nicba1010 commented 1 year ago

403 forbidden, did you check under Settings->Action->General->Workflow permissions->read&write

luigi311 commented 1 year ago

yeah i have the workflow permissions set to read and write permissions. I wonder if its because its a PR it doesnt allow it image

Nicba1010 commented 1 year ago

I opened a test PR in my repo so let's see https://github.com/Nicba1010/JellyPlex-Watched/pull/1

Nicba1010 commented 1 year ago

Weird, it works there

luigi311 commented 1 year ago

hmmm i thought maybe it wouldnt be able to create a package repo if it didnt exist so i pushed a test tag manually to it to see if that works and it does but the action still fails

luigi311 commented 1 year ago

let me merge this in and play with it and see if i can get it working

luigi311 commented 1 year ago

@Nicba1010 It worked once i merged it in.....