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

Revert "Remove failing docker publish line" #64

Merged
merged 7 commits into from
Jul 11, 2023
Merged

Conversation

MSevey
Copy link
Member

@MSevey MSevey commented Jul 7, 2023

Reverts #62

and fixes logic to properly address the issue.

@MSevey MSevey requested a review from a team as a code owner July 7, 2023 15:02
@MSevey MSevey enabled auto-merge (squash) July 7, 2023 16:56
Copy link
Member

@smuu smuu left a comment

Choose a reason for hiding this comment

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

This extra step was added for two reasons:

  • Having amd container images available on all pushed to the repo (Not only pull requests but also on branches, like main)
  • Speeding up the container image publish time. Building the image in arm and amd at the same time takes way too long. When the team publishes a new tag, the container image should be available as soon as possible. And as we only used the amd version of the image, this solution was sufficient enough.

With this change, we no longer have a container image on each push to the repository, and we don't speed up the builds for tags.

@MSevey MSevey requested a review from smuu July 10, 2023 13:47
Bidon15
Bidon15 previously approved these changes Jul 10, 2023
smuu
smuu previously requested changes Jul 10, 2023
Copy link
Member

@smuu smuu left a comment

Choose a reason for hiding this comment

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

Build and Push Docker Images arm64 will override the image pushed from Build and Push Docker Image amd64, resulting that the amd64 version is not available.

@tty47
Copy link
Contributor

tty47 commented Jul 10, 2023

Build and Push Docker Images arm64 will override the image pushed from Build and Push Docker Image amd64, resulting that the amd64 version is not available.

Yes, be careful with that, we faced this issue before and we missed some tags

@MSevey
Copy link
Member Author

MSevey commented Jul 10, 2023

Build and Push Docker Images arm64 will override the image pushed from Build and Push Docker Image amd64, resulting that the amd64 version is not available.

Yes, be careful with that, we faced this issue before and we missed some tags

I think the best course of action is to dumb down this action then.

This action should just build and push a single image. That image and the decision to push should be supplied by the calling repo.

This would then allow each repo to customize which images they are building and when they are pushing them.

All the issues seem to be stemming from us trying to do too many things within the one workflow call. So having the repos have multiple workflow calls seem like the best option.

Thougts?

@smuu
Copy link
Member

smuu commented Jul 11, 2023

Build and Push Docker Images arm64 will override the image pushed from Build and Push Docker Image amd64, resulting that the amd64 version is not available.

Yes, be careful with that, we faced this issue before and we missed some tags

I think the best course of action is to dumb down this action then.

This action should just build and push a single image. That image and the decision to push should be supplied by the calling repo.

This would then allow each repo to customize which images they are building and when they are pushing them.

All the issues seem to be stemming from us trying to do too many things within the one workflow call. So having the repos have multiple workflow calls seem like the best option.

Thougts?

My take:

Teams don't want to handle this, they just want to release their software. The DevOps team owns that (not saying that this is right), and it's easier to control it in one place.
I think using the workflow previous to the changes and adding the check you found (repo name equals to parent repo name) should solve the issue.
This way, we are still able to build images on all commits (for usage with e.g. knuu), and we speedup the process until the amd version of the image is available. Because when the team creates a new tag/release they want to deploy that to Robusta as soon as possible. And building arm in the current way takes a long time. This is the reason that we split that into two jobs. The first one only builds the amd image so that this is available very fast to be able to be used. The second start after that and builds all architectures we want (amd and arm currently) and overrides the previously pushed image so that multiple architectures are available with the same tag.

@MSevey MSevey requested review from Bidon15 and smuu July 11, 2023 13:46
Copy link
Member

@Bidon15 Bidon15 left a comment

Choose a reason for hiding this comment

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

ack

Copy link
Contributor

@tty47 tty47 left a comment

Choose a reason for hiding this comment

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

🚀

@MSevey MSevey dismissed smuu’s stale review July 11, 2023 16:36

Resolved offline

@MSevey MSevey merged commit 84d7d3c into main Jul 11, 2023
7 checks passed
@MSevey MSevey deleted the revert-62-MSevey-patch-1 branch July 11, 2023 16:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants