-
Notifications
You must be signed in to change notification settings - Fork 65
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
ci: add docker build action #549
Conversation
Yep cargo dist can be removed completely. |
@IgnisDa I think our |
The build successed on my end (https://github.com/vnghia/ryot/pkgs/container/ryot/162832490?tag=pr-3), this is probably because I don't have permission to write to this repo yet and the PR isn't merged. Pushing to |
277a2ac
to
9235d4b
Compare
9235d4b
to
db4b54a
Compare
.github/workflows/release.yml
Outdated
# Mark the Github Release™ as a non-draft now that everything has succeeded! | ||
publish-release: | ||
# Only run after all the other tasks, but it's ok if upload-artifacts was skipped | ||
needs: | ||
[create-release, upload-artifacts, docker-release, upload-kodi-plugin] | ||
[create-release, upload-artifacts, upload-kodi-plugin] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we do away with cargo-dist in this release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, what do you mean by "do away" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I got it, never heard it before. The reason why I haven't touched it is because I don't have enough information about what it is doing, espcially the upload-kodi-plugin
. Also the deploy and the build docs should probably be kept so what do you think about these two jobs ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep deploy
updates the public instance and upload-kodi-plugin
creates the kodi archive. Both of the jobs are required.
This PR also addresses #544 (comment) |
Help me understand this PR. When I push a new tag, the If this is the case the only jobs in |
Do you want the fly deploy to use the latest release or the latest commit to main ? |
The latest commit, what we tag as |
I am not sure how we should handle the ci logic without |
@IgnisDa, as far as I can tell, github will internally create a branch everytime there is a new PR. So when I do a release commit for testing on my fork, it will also make that commit a release on your end. It seems bypassing whatever permission model about the CI as well and this is a security issue with github and I will try to report it as soons as possible. |
It seems that you are the one who pushing the release commit, not me and thus bypassing the permission checking. Maybe because you are editing directly my PR so github counts that commit as yours. |
Ah well, that makes sense. I assumed that only workflows committed to |
@IgnisDa, and my last suggestion: switch from and
|
I've just made a tag here: https://github.com/vnghia/ryot/releases/tag/v4.2.0 |
@vnghia Do you think this is ready to be merged? |
I think it's ready to be merged 🥳 |
b1fafd4
to
7bee441
Compare
@vnghia I am not able to figure out how to publish the Can't have that happening since that will prevent all existing installations of Ryot (that use |
Something like this should work for the publish to ghcr step:
I'm not sure if you can omit the ghcr.io/user/repo, but this is how I have it on my private repo. |
@Robin-Sch I want this workflow to make a |
If a PR is made do you want a new docker version to be released at all? And do you want it to update the public fly instance? You could make 2 seperate workflows, one for whatever you want on a PR, and one for whatever you want on a push. |
On PRs I want a release called Would these requirements mean I have to create 2 different workflows? Won't that result in a lot of duplicated code? |
I'm not sure how you'd do it otherwise, maybe @vnghia could help with that? EDIT: and add something like:
But I'm not sure what that XXXXX variable would be. Also looking at the same workflow step, it seems like the current setup will also update vX.X.X whenever a new commit is made (at the same time that unstable is updated), as they use the same enable variable. Which I don't think is what you want? |
True, I do not want it. @vnghia Could you please help here? |
I am a little busy right now. I will try on my fork next week! |
Did you manage to find a solution? |
Hey @vnghia, any updates on this? |
I will be back next week. Sorry for the delay 😅 |
Closing in favour of #770. |
Part of #544
This PR adds a docker pipeline for PR, and push to main. Since we are no longer supporting bare-metal installation, I think we should probably just remove
cargo-dist
entirely and only focusing on Docker. What do you think @IgnisDa ?