-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: add automated container build #108
base: master
Are you sure you want to change the base?
Conversation
Hi @sxd Thank you, I will have a look at it over the weekend. |
Hi @sxd First review of your PR looks promising. I do however have some obstacles I have to overcome first. I need to get the required authorizations to work in a balanced and secure manner, so this will require some work before the PR can be processed and approved. The challenges are:
All of the above is not caused by your PR, but it was a question of time before these decisions had to be made. So please bear with me and I will get back to you |
hi @jonasbn Using GHCR makes a lot of sense and doesn't require too much changes to my PR. Cheers! |
Hi @rojopolis Could you perhaps help us out here. We need to have the permissions associated with the auto-generated Currently we are using DockerHub, but with this PR I believe it makes more sense to change to
The checkbox "Allow GitHub actions to create and approve pull requests should not be ticked. REF: GitHub Docs |
@jonasbn hi! I don't think we need to enable it, probably is enabled by default, we can just create and push the image, we can actually try that, what do you think? Regards! |
HI Jonas,
Sure!
I’m away right now (and am forgetful so please ping me if I haven’t done it by Monday).
Cheers,
Robert
… On Aug 5, 2022, at 7:29 AM, Jonathan Gonzalez V. ***@***.***> wrote:
@jonasbn <https://github.com/jonasbn> hi!
I don't think we need to enable it, probably is enabled by default, we can just create and push the image, we can actually try that, what do you think?
Regards!
—
Reply to this email directly, view it on GitHub <#108 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJMYNL6IEGPFIL6NP4TZU5TVXUQOJANCNFSM53TYMRYQ>.
You are receiving this because you were mentioned.
|
Oh, sorry… I didn’t see the second message…
Is there a cost for the image repo?
… On Aug 5, 2022, at 8:01 AM, Robert Jordan ***@***.***> wrote:
HI Jonas,
Sure!
I’m away right now (and am forgetful so please ping me if I haven’t done it by Monday).
Cheers,
Robert
> On Aug 5, 2022, at 7:29 AM, Jonathan Gonzalez V. ***@***.*** ***@***.***>> wrote:
>
>
> @jonasbn <https://github.com/jonasbn> hi!
>
> I don't think we need to enable it, probably is enabled by default, we can just create and push the image, we can actually try that, what do you think?
>
> Regards!
>
> —
> Reply to this email directly, view it on GitHub <#108 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AJMYNL6IEGPFIL6NP4TZU5TVXUQOJANCNFSM53TYMRYQ>.
> You are receiving this because you were mentioned.
>
|
Hi @rojopolis No no it's free, it might be enabled already. So together with @sxd I will do some experimentation and validation. So do not sweat it, I will ping you if we need you assistance. Thanks |
I did a run on the PR. If failed with the following:
Ref: line 668 of the "Build container" step. Any ideas? |
I found this older issue googling: docker/build-push-action/issues/463 It is the same diagnostics, so the suggested remedy might be the same. |
@jonasbn I had to deal with that issue yesterday, it's about the permissions in the action like here https://github.com/cloudnative-pg/webtest/blob/main/.github/workflows/ci.yml#L15 I'm will go out for lunch in a couple of minutes and I'll get back to take a look! and check and fix it! never mind, just did it was quite fast :P |
@jonasbn can you trigger the run again? I'll check it later :D |
@sxd same outcome:
|
@jonasbn the permissions is not there for the packages :S https://github.com/rojopolis/spellcheck-github-actions/runs/7695549439?check_suite_focus=true#step:1:19 can you try again ? I pushed some small changes, but yes it's weird the permissions were not there |
Same again is not even using the proper commit :S https://github.com/rojopolis/spellcheck-github-actions/runs/7695700457?check_suite_focus=true#step:2:138 |
@jonasbn weird, you can see that it's working here https://github.com/sxd/spellcheck-github-actions/actions/runs/2805064755 :S |
@sxd I will get @rojopolis to help evaluate the settings based on the reference I located, I believe this will get it to work, I am still think this is related to permissions. |
@jonasbn totally agree @rojopolis can you give @jonasbn admin permissions on the repo for a while at least so he can properly configure the repo? |
I don't believe I can because this repo doesn't belong to an Organization. https://docs.github.com/en/account-and-profile/setting-up-and-managing-your-personal-account-on-github/managing-personal-account-settings/permission-levels-for-a-personal-account-repository#about-permissions-levels-for-a-personal-account-repository |
@rojopolis @jonasbn yes! probably that's the issue we faced the same a couple of weeks ago and now we throw the test using pull_request_target, so the package it's ok, it should fail since it's running in a forked repo, that will not change even if we change it in the PR. So, the only way to test this will be to change the CI to use pull_request_target first, instead of using pull_request |
Use the `docker/build-push` and `docker/metadata` actions to build a container and add the proper tag depending on the branch, PR or the tagged version using semver as the proper version. Closes rojopolis#80 Signed-off-by: Jonathan Gonzalez V <[email protected]>
Use the
docker/build-push
anddocker/metadata
actions to builda container and add the proper tag depending on the branch, PR or
the tagged version using semver as the proper version.
Closes #80
Signed-off-by: Jonathan Gonzalez V [email protected]