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

GHA: Docker: don't run on support/* branches #758

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Al2Klimov
Copy link
Member

PRs to them are already covered and we don't need support* image tags.

PRs to them are already covered and we don't need support* image tags.
@cla-bot cla-bot bot added the cla/signed label May 28, 2024
@julianbrost
Copy link
Contributor

we don't need support* image tags

So the main goal of this PR to not push them to Docker Hub anymore?

PRs to them are already covered

Partially. We don't require that PRs are rebased on the latest version on the target branch, so there's a small chance that merging an older PR results in an issue on the target branch.

@Al2Klimov
Copy link
Member Author

we don't need support* image tags

So the main goal of this PR to not push them to Docker Hub anymore?

👍

PRs to them are already covered

Partially. We don't require that PRs are rebased on the latest version on the target branch, so there's a small chance that merging an older PR results in an issue on the target branch.

The chance that such issue occurs only in the Docker GHA is even smaller.

@julianbrost
Copy link
Contributor

That sounds more like a workaround for limitations in Icinga/docker-icingadb, which is a project we control, so why should we implement a workaround here?

Though that raises the question whether we want to fix something in Icinga/docker-icingadb or rather just move towards an in-project Dockerfile with a workflow using the standard docker/* actions like we already started doing in Icinga Notifications (https://github.com/Icinga/icinga-notifications/blob/main/Dockerfile, https://github.com/Icinga/icinga-notifications/blob/main/.github/workflows/docker.yml) and Icinga for Kubernetes (https://github.com/Icinga/icinga-kubernetes/blob/main/Containerfile, https://github.com/Icinga/icinga-kubernetes/blob/main/.github/workflows/docker-image.yml).

@Al2Klimov
Copy link
Member Author

@lippserd
Copy link
Member

lippserd commented Oct 2, 2024

To be honest, I don't quite understand your discussion as images are really not needed for support branches and a similar PR has already been merged in Icinga 2. But I welcome moving the Dockerfile here, which still requires environment variable support to be merged, but that is something that is definitely coming.

@Al2Klimov please answer #758 (comment). I'm also wondering about the "we don't need support* image tags" wording. These branches are supposed to have tags for which an image should be built and published. But I think you mean that we tag the image with the name of the support branch? In that case, I would just write "images for support branches". Anyway, please clarify.

@julianbrost any objections to just merging?

@julianbrost
Copy link
Contributor

@julianbrost any objections to just merging?

Not really. You'd probably have to try very hard to introduce a problem that was only caught by that single job. Still, I find it strange that we have to remove the whole job just to prevent the upload. I'd just find it much nicer if every single commit would run the same set of checks before and after merging, avoiding the whole "is this job strictly necessary in this situation" consideration.

@lippserd
Copy link
Member

lippserd commented Oct 2, 2024

@julianbrost any objections to just merging?

Not really. You'd probably have to try very hard to introduce a problem that was only caught by that single job. Still, I find it strange that we have to remove the whole job just to prevent the upload. I'd just find it much nicer if every single commit would run the same set of checks before and after merging, avoiding the whole "is this job strictly necessary in this situation" consideration.

Sorry to ask again 😆, but I think I have understood the point: You still want to create the image, but just not push it, right? If so, I second that, but I would like to cover this when moving the Dockerfile here.

@julianbrost
Copy link
Contributor

julianbrost commented Oct 2, 2024

You still want to create the image, but just not push it, right?

Yes, exactly. For all the other jobs we don't ask the question "do we need the build artifacts for anything afterwards" either. And my intuition says that adding a simple if: to the push step would be preferred, the problem is just that with the current icinga/docker-icingadb project structure, that would be a more complex cross-repo change for something that we plan to phase out. So if the container stuff is moved back into this repo, I'd say there's a good chance the job is run again on the support/* branches, just without pushing the image anywhere.

@Al2Klimov
Copy link
Member Author

images are really not needed for support branches and a similar PR has already been merged in Icinga 2.

Exactly my opinion.

I'm also wondering about the "we don't need support* image tags" wording. These branches are supposed to have tags for which an image should be built and published. But I think you mean that we tag the image with the name of the support branch?

Exactly what I meant. This PR is just about for which branches that GHA runs and pushes images named after the respective branch. It has nothing to do with tags.

any objections to just merging?

Not really.

Well, then...

@Al2Klimov
Copy link
Member Author

You'd probably have to try very hard to introduce a problem that was only caught by that single job.

If we break it intentionally this way, we deserve it. :-)

@yhabteab yhabteab removed their assignment Oct 28, 2024
@yhabteab yhabteab removed their request for review October 28, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants