anatol / pacoloco

Caching proxy server for Arch Linux pacman
MIT License
216 stars 30 forks source link

Sanitize image name in workflow #97

Closed Orochimarufan closed 10 months ago

Orochimarufan commented 11 months ago

The push workflow breaks on forks with any uppercase characters in either the user/org or repo name. This is because container registries require that the image refs are all lowercase, and while docker/metadata-action usually handles that for generated tags, the multi-arch build juggles the images around a bit before assigning a tag, so it uses the raw image name derived from the qualified repo name.

This adds steps to explicitly convert the image name to lowercase. Additionally, it includes 2 minor changes to the workflow:

I will drop the latter if you think it's a bad idea, but it seems useful for easier testing of branches. Besides that, I'd suggest reverting to the docker actions' default behavior of having :latest track the latest release, while :master can track all commits to master. While there hasn't been a release yet since the container images were added, having :latest track releases is more useful for end-users (and having tags for branches is nice for devs).

anatol commented 11 months ago

cc @dezeroku @zhulik for this PR review

dezeroku commented 11 months ago

I like it, nice job!

I wonder if we should also use the lowercased names for docker/metadata-action action, but if it works fine with the mixed usage there then it's ok for me.

As for tagging the branches, I think it's a good idea, keep it in.

I'd usually expect the :latest to point to the last commit on the main branch, so maybe something like :stable for the releases (with a note in the README)? I'd like to hear more voices here

As for the review, LGTM

dezeroku commented 11 months ago

BTW, it's true that we don't have any tagged releases with docker images yet (take a look here for a list) and there's only one code-changing commit added since the docker push flow was changed.

@anatol, maybe we could consider doing a small fix release (as in 1.5.1) after this is merged, so it could be properly locked in by the end users?

anatol commented 11 months ago

Sure, I'll be glad to make a new release once this PR merged.

anatol commented 10 months ago

Thank you for the contribution @Orochimarufan!

I plan to tag pacoloco soon. @dezeroku @Orochimarufan do you want to check master branch before I create a new tag?

dezeroku commented 10 months ago

Current master works good on my end (arm64). Green light from me

Orochimarufan commented 10 months ago

Current master seems to have built fine, so all good as far as the tech goes. We haven't really come to a conclusion on how :latest should be tagged though.

Looking around DockerHub, :latest tends to be a kind of 'rolling stable' for most images. I suppose a lot of projects also have a stable master branch (with development happening in a separate develop branch) so it may still end up tracking master, but IMHO having :latest be unfiltered dev builds is unexpected. It's also less useful since we have branch-tracking tags now (including :master) but there isn't currently any way to specify "the latest tagged release".

In the end, you can decide either way. If you want to move :latest to releases, I can prepare another quick PR, if not I'll drop it. I also have some actual code changes to submit now that this is merged.

anatol commented 10 months ago

Alright, pacoloco head has been tagged with 1.6.