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

add multi arch docker build #5808

Merged
merged 36 commits into from Nov 5, 2023
Merged

add multi arch docker build #5808

merged 36 commits into from Nov 5, 2023

Conversation

b-reich
Copy link
Contributor

@b-reich b-reich commented Nov 2, 2023

Closes #4089

Changes proposed in this pull request:

  • use buildx for docker building
  • use github action for building

How to test the feature manually:

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard)
  • documentation updated

Additional information can be found in the documentation.

@b-reich b-reich changed the title add multi arch build add multi arch docker build Nov 2, 2023
@b-reich
Copy link
Contributor Author

b-reich commented Nov 2, 2023

Is publishing on github actions wantend?

@b-reich
Copy link
Contributor Author

b-reich commented Nov 2, 2023

https://github.com/b-reich/FreshRSS/actions/runs/6733089164/job/18301035008

looks like the build itself works

@Alkarex Alkarex added the Docker Everything related to Docker label Nov 2, 2023
@Alkarex Alkarex added this to the 1.23.0 milestone Nov 2, 2023
@Alkarex
Copy link
Member

Alkarex commented Nov 2, 2023

Thanks, that looks pretty good already 👍🏻
I am having a look

@Alkarex
Copy link
Member

Alkarex commented Nov 2, 2023

For reference, the current Docker Hub build configuration:

image

@b-reich
Copy link
Contributor Author

b-reich commented Nov 2, 2023

For reference, the current Docker Hub build configuration:

image

in the end we should only have something like latest and latest-alpine i think. No need to tag the arm images separately. This is kinda a breaking change than.

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Nov 2, 2023

Maybe the Dockerfile Labels should also be adapted for this. The docker/metadata-action sets a bunch of labels by itself so we shouldn't duplicate ourselves here?

LABEL \
org.opencontainers.image.authors="Alkarex" \
org.opencontainers.image.description="A self-hosted RSS feed aggregator" \
org.opencontainers.image.documentation="https://freshrss.github.io/FreshRSS/" \
org.opencontainers.image.licenses="AGPL-3.0" \
org.opencontainers.image.revision="${SOURCE_BRANCH}.${SOURCE_COMMIT}" \
org.opencontainers.image.source="https://github.com/FreshRSS/FreshRSS" \
org.opencontainers.image.title="FreshRSS" \
org.opencontainers.image.url="https://freshrss.org/" \
org.opencontainers.image.vendor="FreshRSS" \
org.opencontainers.image.version="$FRESHRSS_VERSION"

@b-reich
Copy link
Contributor Author

b-reich commented Nov 2, 2023

Maybe the Dockerfile Labels should also be adapted for this. The docker/metadata-action sets a bunch of labels by itself so we shouldn't duplicate ourselves here?

LABEL \
org.opencontainers.image.authors="Alkarex" \
org.opencontainers.image.description="A self-hosted RSS feed aggregator" \
org.opencontainers.image.documentation="https://freshrss.github.io/FreshRSS/" \
org.opencontainers.image.licenses="AGPL-3.0" \
org.opencontainers.image.revision="${SOURCE_BRANCH}.${SOURCE_COMMIT}" \
org.opencontainers.image.source="https://github.com/FreshRSS/FreshRSS" \
org.opencontainers.image.title="FreshRSS" \
org.opencontainers.image.url="https://freshrss.org/" \
org.opencontainers.image.vendor="FreshRSS" \
org.opencontainers.image.version="$FRESHRSS_VERSION"

The generated one from the metadata action are not used at the moment. this could be change @Alkarex what do you think?

@Alkarex
Copy link
Member

Alkarex commented Nov 2, 2023

No need to tag the arm images separately.

Indeed. I think it would be an acceptable breaking change.

Why multiple prebuilt images?

Providing a Debian and an Alpine version is quite common. There are pros and cons with both (for instance build speed for Alpine, among other things), and having an alternative can be helpful (e.g. #5742 )

I will come back for the other questions

@b-reich
Copy link
Contributor Author

b-reich commented Nov 2, 2023

https://github.com/b-reich/FreshRSS/actions/runs/6738089530/job/18316933656

the github action is nearly finish I think....
How to handle the tags is a thing now.

Co-authored-by: EdJoPaTo <rfc-conform-git-commit-email@funny-long-domain-label-everyone-hates-as-it-is-too-long.edjopato.de>
Co-authored-by: Alexandre Alapetite <alexandre@alapetite.fr>
@Alkarex
Copy link
Member

Alkarex commented Nov 2, 2023

The generated one from the metadata action are not used at the moment. this could be change @Alkarex what do you think?

Yes, it would probably be better to use the ones from the Action, indeed

@b-reich b-reich marked this pull request as ready for review November 3, 2023 06:52
Comment on lines 26 to 36
LABEL \
org.opencontainers.image.authors="Alkarex" \
org.opencontainers.image.description="A self-hosted RSS feed aggregator" \
org.opencontainers.image.documentation="https://freshrss.github.io/FreshRSS/" \
org.opencontainers.image.licenses="AGPL-3.0" \
org.opencontainers.image.revision="${SOURCE_BRANCH}.${SOURCE_COMMIT}" \
org.opencontainers.image.source="https://github.com/FreshRSS/FreshRSS" \
org.opencontainers.image.title="FreshRSS" \
org.opencontainers.image.url="https://freshrss.org/" \
org.opencontainers.image.vendor="FreshRSS" \
org.opencontainers.image.version="$FRESHRSS_VERSION"
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to keep the static ones here, so that they exist when the builds are done locally, e.g. like

build:
# Pick #latest (stable release) or #edge (rolling release) or a specific release like #1.21.0
context: https://github.com/FreshRSS/FreshRSS.git#latest
dockerfile: Docker/Dockerfile-Alpine

authors can be removed

By the way, if anyone knows a way to populate revision and/or version even in those more manual scenarios, let me know

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure what happen when you use the github action ones and the static ones.

Copy link
Member

Choose a reason for hiding this comment

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

Let's try to remove the static ones from GitHub Actions, so that we can see whether that is sufficient or whether they need to be overridden again

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I found some docu about.
passing labels at build time will overwrite the static ones

# username: ${{ secrets.DOCKERHUB_USERNAME }}
# password: ${{ secrets.DOCKERHUB_TOKEN }}

# - name: Login to GitHub Container Registry
Copy link
Member

Choose a reason for hiding this comment

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

Any comments about how pros and cons of (also) using the GitHub Container Registry?
Probably seems overkill for now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For podman user?
And Dockerhub had in the pass some weird decisions I think.

Personaly I prefer the github registry

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Let's keep it but comment out for now, so we can easily activate if needed.
I am not familiar with the use-case: Can't Podman users pull from Docker Hub?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It can, but the default registry (dockerhub) is not set.

Copy link
Contributor

Choose a reason for hiding this comment

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

Newer podman versions try dockerhub on unspecified image tags. Personally I don't think it matters much as its the same image

@b-reich
Copy link
Contributor Author

b-reich commented Nov 5, 2023

@Alkarex you need to checkout the repo when you try to retrive the version

@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2023

you need to checkout the repo when you try to retrive the version

Thanks. Right, I had read a bit too fast that comment #5808 (comment)

As you can see, I am experimenting with an arg-based approach for labels. What do you think?

@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2023

Hum, I was hopping that docker/metadata-action could be skipped, but tags: ${{ matrix.tags }} does not seem to work directly...

@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2023

The output looks good to me now, although I would have liked to simplify a bit more if we could avoid docker/metadata-action.
Final thoughts @b-reich ?

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Nov 5, 2023

Out of curiousity: why would you like to avoid using metadata-action? it seems helpful to me for generating the v1, v1.2, v1.2.3 tags and so on. Maybe I overlook something?

tags: ${{ matrix.tags }}
labels: |
org.opencontainers.image.revision=${{ github.ref_name }}.${{ github.sha }}
org.opencontainers.image.version=${{ env.FRESHRSS_VERSION }}
Copy link
Contributor Author

@b-reich b-reich Nov 5, 2023

Choose a reason for hiding this comment

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

this is already handle by the org.opencontainers.image.revision label which is the commit hash
dont know if there is benefit to know which branch is build from, when you have the commit

also thinks the container version label is fine with sometthing like 1.22.2 or 1.22.2-alpineits specify the container version not the version of the application itself

Copy link
Member

@Alkarex Alkarex Nov 5, 2023

Choose a reason for hiding this comment

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

if there is benefit to know which branch is build from

This is mostly because this is what we already have, and I find it a bit easier to read with something like 1.22.1.cc2878aed855463efe36cf6fb96d001dbb75612e. But I would not be much against leaving it to only the git SHA, if this is what is most standard.

Edit: indeed, according to https://specs.opencontainers.org/image-spec/annotations/#pre-defined-annotation-keys : org.opencontainers.image.revision Source control revision identifier for the packaged software.
I will edit accordingly

also thinks the container version label is fine with sometthing like 1.22.2 or 1.22.2-alpineits specify the container version not the version of the application itself

I would also be happy with 1.22.2-alpine, but that does not work for e.g. edge builds

Copy link
Member

Choose a reason for hiding this comment

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

Switched back to default revision 3d110b8

@b-reich
Copy link
Contributor Author

b-reich commented Nov 5, 2023

Not sure why you would avoid using the metadata action?
Personally I wouldn't worry so much about the labels. Many people aren't even interested in them.

@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2023

Out of curiousity: why would you like to avoid using metadata-action? it seems helpful to me for generating the v1, v1.2, v1.2.3 tags and so on. Maybe I overlook something?

Although it does not matter much, it would be mostly to remain as simple as possible, as we do not know how things will evolve in the future. For instance, we have been through periods during which the auto-build in Docker Hub was not available while waiting for a (initially long) Sponsored OSS process. During that period, I built and pushed images manually. The more tags there are, the more difficult it is to move to another build system, and the more work there is for the maintenance.

Another use-case: if we were to move back to building on Docker Hubs, there is poor support for multiple tags, which each take a new build, so I would not like to have 1, 1.22, 1.22.1 but only 1.22.1. Especially not when combined with multiple architectures and Debian + Alpine. It just becomes too many tags.

Furthermore, FreshRSS is not a library. And we do not have the resources to maintain several branches anyway. So the semver semantics only marginally applies to it. For instance, fixes in 1.22.1 may very well be in 1.23.0. So I am not sure why anyone would chose to track 1.22 instead of 1.22.1 or latest. And by offering semver tags, it might mislead some users into believing that we do have updates on old branches, which we do not.

Personally I wouldn't worry so much about the labels. Many people aren't even interested in them.

Yes, and that is why I would probably like to err on the side of simplicity instead of having rarely-used features, which might end up having some drawbacks (see my explanation higher up). As soon as we introduce something, I take on me to actually make a good effort to maintain it over time, so I need to balance what we add with its actual value.

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented Nov 5, 2023

Although it does not matter much, it would be mostly to remain as simple as possible, as we do not know how things will evolve in the future. For instance, we have been through periods during which the auto-build in Docker Hub was not available while waiting for a (initially long) Sponsored OSS process. During that period, I built and pushed images manually. The more tags there are, the more difficult it is to move to another build system, and the more work there is for the maintenance.

Another use-case: if we were to move back to building on Docker Hubs, there is poor support for multiple tags, which each take a new build, so I would not like to have 1, 1.22, 1.22.1 but only 1.22.1. Especially not when combined with multiple architectures and Debian + Alpine. It just becomes too many tags.

When I moved away from auto build on Docker Hub it supported multiple tags like 1, 1.2, 1.2.3, … But that is some time ago. I explicitly searched for a way to replicate that behavior on GitHub Actions so I know and understand the pain points you are speaking of. Thanks for clarifying :)

Furthermore, FreshRSS is not a library. And we do not have the resources to maintain several branches anyway. So the semver semantics only marginally applies to it. For instance, fixes in 1.22.1 may very well be in 1.23.0. So I am not sure why anyone would chose to track 1.22 instead of 1.22.1 or latest. And by offering semver tags, it might mislead some users into believing that we do have updates on old branches, which we do not.

Personally I use the main version tag for my containers. I get all the new features and bugfixed without worrying about breaking changes (new major release). Chosing a minor release would be to manually update but get bugfix only releases which don't change much and update the rest manually (like you suggested pinning a git hash on a used action.

Just to give an idea here: Maybe only support major and bugfix version (1 and 1.2.3). Minor releases and bugfix releases would be kinda the same. When someone wants to pin them they can. If they only care for breaking changes they can use the major version tag. This reduces the amount of tags while still allowing for non-breaking tags like I mainly use.
After all its only a single line less in the metadata-action config.

Might be enabled later if desired
@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2023

Maybe only support major and bugfix version (1 and 1.2.3)

Ok, let's go for that

@Alkarex Alkarex merged commit 8f07199 into FreshRSS:edge Nov 5, 2023
1 check passed
@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2023

Let's give it a spin 🚀
Thanks for your patience and constructive dialogue so far 👍🏻

Seems to work fine for most of it:
https://github.com/FreshRSS/FreshRSS/actions/runs/6763929607/job/18381624080

image

image

There are only a few things that need to be ironed out:
https://github.com/FreshRSS/FreshRSS/actions/runs/6763929609/job/18381624094

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Nov 5, 2023
@Alkarex Alkarex mentioned this pull request Nov 5, 2023
@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2023

@b-reich and @EdJoPaTo : please add a line for you in https://github.com/FreshRSS/FreshRSS/blob/edge/CREDITS.md

Alkarex added a commit that referenced this pull request Nov 5, 2023
@Alkarex
Copy link
Member

Alkarex commented Nov 5, 2023

Everything seems fine now, including the push to the Docker Hub description https://hub.docker.com/r/freshrss/freshrss/

Ideas for future improvement:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Docker Everything related to Docker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Docker] Support for raspberry pi OS 64 bit
3 participants