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

(refactor): optimize Dockerfiles, parallel build process #84

Merged
merged 6 commits into from
Dec 11, 2023

Conversation

traktuner
Copy link
Collaborator

@traktuner traktuner commented Dec 11, 2023

Changes in this PR:

-) Used new jlesage/baseimage-gui v4 (the image without version tag is not maintained anymore)
-) the Dockerfiles are synchronized now (only difference is the used baseimage)
-) the images now get built in parallel with GitHub Actions*

*) please add a new GitGub actions variable DOCKERHUB_USER to your repo with your username before merging. I wanted to test the build and so I had to use variables instead of a hard coded username 😉

@traktuner
Copy link
Collaborator Author

@JonathanTreffler If you're interested, I'd be happy if you could test those changes.
I can also add a Dockerfile with the newly released ubuntu-22 baseimage from jlesage. Already tested it myself.
Please let me know - thanks 😊

@JonathanTreffler
Copy link
Owner

JonathanTreffler commented Dec 11, 2023

Thank you for the PR :)

I will review the rest, but before that a small note:

-) a new APP_ICON was added

The reason the existing APP_ICON code is commented out is, that I do not have any rights to Backblaze's Logo and I have no idea if I would be allowed to include it in the Docker Image. And I just don't think it is important enough to risk legal trouble :)

If you could please take out the APP_ICON from the PR, that would be nice, otherwise I will just merge it as is and remove it on my main branch. Thx :)

@traktuner traktuner changed the title (refactor): optimize Dockerfiles, new icon, parallel build process (refactor): optimize Dockerfiles, parallel build process Dec 11, 2023
@traktuner traktuner marked this pull request as ready for review December 11, 2023 14:16
@traktuner
Copy link
Collaborator Author

Ah got it - thanks :)
I removed the icon!
The checks are currently failing because of the missing DOCKERHUB_USER GitHub actions variable (I needed those to test the build in my environment).

@JonathanTreffler
Copy link
Owner

I can also add a Dockerfile with the newly released ubuntu-22 baseimage from jlesage. Already tested it myself.

That would be great, thank you :)

I think with all the weird "needing ubuntu 18 for some reason on some devices, nobody knows why" stuff it would be good to stick with ubuntu 20 as the base for the latest image tag for now, but adding the v22 as an extra (non-versioned) tag like ubuntu18 would be great, so people can test it out and give feedback, before we migrate everybody else on latest over to it.

@traktuner
Copy link
Collaborator Author

Sure, I will do that in a separate PR!
I was one of those "need ubuntu18" users :D
With ubuntu20, a connection to backblaze was not possible. But it seems that some Synology DSM updates fixed things, now ubuntu20 and the ubuntu22 images work great!

@JonathanTreffler
Copy link
Owner

The checks are currently failing because of the missing DOCKERHUB_USER GitHub actions variable (I needed those to test the build in my environment).

I already added the env and restarted the action, but they are not used here and the docs explain why:

Anyone with collaborator access to this repository can use these secrets and variables for actions. They are not passed to workflows that are triggered by a pull request from a fork.

I invited you to be a repository collaborator, so you can develop from a branch instead of a fork if you want :)

@JonathanTreffler JonathanTreffler self-requested a review December 11, 2023 14:50
Copy link
Owner

@JonathanTreffler JonathanTreffler left a comment

Choose a reason for hiding this comment

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

LGTM :)

@JonathanTreffler
Copy link
Owner

I reviewed it so now all prerequisites for a merge should be fullfilled, you should be able to merge it yourself :)

@traktuner traktuner merged commit 401bb7f into JonathanTreffler:main Dec 11, 2023
1 of 3 checks passed
@JonathanTreffler
Copy link
Owner

The new actions worked beautifully, the main and ubuntu18 branches are updated and the latest and versioned branches should get updated with the next versioned release.

Screenshot 2023-12-11 160211

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