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

Improve docker build workflow in forks #337

Merged
merged 3 commits into from
Nov 27, 2024
Merged

Conversation

isZumpo
Copy link
Contributor

@isZumpo isZumpo commented Nov 26, 2024

Description of changes:
When forking the repository for development purposes, the github action workflow inside the fork will try upload the built image to ghcr.io/ionos-cloud/cluster-api-provider-proxmox, which results in a permission error: ERROR: denied: permission_denied: The requested installation does not exist.

This can be avoided by instead of having a hardcoded image name in the workflow, one dynamically takes the GITHUB_REPOSITORY variable and converts it into lowercase (for those who have Github usernames with one or more capital letters). Consequentially, every fork will push to its own registry without permission issues, while the main repository keeps working just like before.

Testing performed:
Verified workflow in my fork. It manages to publish ghcr images to my fork/registry.

Instead of using hardcoded image name, take it from the repo name. This makes it work well for forks.
Copy link

🚀 e2e tests run

We add labels to the PRs to control the e2e test runs by running specific tests and skipping some test contexts,
please follow this guide:

Label Behaviour
none Run Generic tests
e2e/none skip all e2e tests (documentation etc) - overrides all e2e/* labels Do not run any tests (overrides all e2e/ flags)
e2e/flatcar run Flatcar e2e tests Add Flatcar tests to the run

ℹ️ Ask a Member to add the requested labels if you don't have enough permissions.

Copy link
Member

@mcbenjemaa mcbenjemaa left a comment

Choose a reason for hiding this comment

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

it's better to keep this in your Fork.
our repo will not change for now.

@pborn-ionos
Copy link
Contributor

@mcbenjemaa i have to disagree with you here. I think the proposed change makes a lot of sense, since it allows contributors to more easily verify their changes in their repos, before submitting them to us.

mcbenjemaa
mcbenjemaa previously approved these changes Nov 27, 2024
Copy link
Member

@mcbenjemaa mcbenjemaa left a comment

Choose a reason for hiding this comment

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

LGTM

@mcbenjemaa mcbenjemaa added the e2e/none skip all e2e tests (documentation etc) - overrides all e2e/* labels label Nov 27, 2024
@mcbenjemaa
Copy link
Member

@isZumpo can you check the actionlint

Copy link
Collaborator

@wikkyk wikkyk left a comment

Choose a reason for hiding this comment

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

Just this minor thing. Aside from that, I'm happy for this to go in.

.github/workflows/container-image.yaml Outdated Show resolved Hide resolved
@mcbenjemaa mcbenjemaa enabled auto-merge (squash) November 27, 2024 15:45
@mcbenjemaa mcbenjemaa disabled auto-merge November 27, 2024 15:45
@mcbenjemaa mcbenjemaa merged commit 51a9d0a into ionos-cloud:main Nov 27, 2024
8 checks passed
@isZumpo
Copy link
Contributor Author

isZumpo commented Nov 27, 2024

Glad to see that the PR managed to get positive feedback and fixes commited and even a merge. All while I was away at work. Great work and thanks to everyone 😁

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
e2e/none skip all e2e tests (documentation etc) - overrides all e2e/* labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants