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

Add an architecture suffix to images pushed for multi-platform if missing #1185

Merged
merged 1 commit into from
Aug 12, 2024

Conversation

arewm
Copy link
Member

@arewm arewm commented Jul 23, 2024

In order to reduce the likelihood of users accidentally forgetting to specify unique tags for each architecture, we can add a suffix to the pushed image

Before you complete this pull request ...

Look for any open pull requests in the repository with the title "e2e-tests update" and
see if there are recent e2e-tests updates that will be applicable to your change.

@arewm arewm force-pushed the add-arch-to-multi-platform-images branch 8 times, most recently from 28d8820 to d0ae2cd Compare July 23, 2024 19:56
@arewm arewm changed the title Add an architecture suffix to images pushed for multi-platform WIP: Add an architecture suffix to images pushed for multi-platform Jul 24, 2024
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch 6 times, most recently from ccbd82e to 6c066ea Compare July 24, 2024 14:26
@arewm arewm marked this pull request as draft July 25, 2024 20:08
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch from 6c066ea to 2ea2ad7 Compare July 26, 2024 16:03
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch 6 times, most recently from d28bb14 to 1ee4628 Compare July 26, 2024 19:50
@arewm arewm marked this pull request as ready for review July 26, 2024 19:50
@arewm arewm changed the title WIP: Add an architecture suffix to images pushed for multi-platform Add an architecture suffix to images pushed for multi-platform if missing Jul 26, 2024
@arewm arewm requested a review from chmeliik July 26, 2024 19:51
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch from 1ee4628 to f886065 Compare July 27, 2024 02:52
task/buildah/0.2/buildah.yaml Outdated Show resolved Hide resolved
task-generator/remote/main.go Show resolved Hide resolved
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch from 8f0c86a to 468b729 Compare July 31, 2024 12:13
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch from 468b729 to 41b1eff Compare July 31, 2024 13:31
@arewm arewm mentioned this pull request Aug 2, 2024
5 tasks
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch from 41b1eff to 824d09a Compare August 5, 2024 13:54
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch 6 times, most recently from af89669 to 39ee314 Compare August 5, 2024 15:21
@arewm arewm requested review from a team and chmeliik August 5, 2024 15:22
task-generator/remote/main.go Outdated Show resolved Hide resolved
step := &task.Spec.Steps[stepPod]
if step.Name != "build" {
if step.Script != "" && taskVersion == "0.2" && step.Name != "build" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm afraid, that supporting many task versions is error prone.
Should we support only latest buildah task version?

Copy link
Member Author

Choose a reason for hiding this comment

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

We cannot easily add this to v0.1 because we don't have IMAGE_REF to fall back on to when we change the image tag that is produced.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we mark the other versions as deprecated?

We did this here.

That makes EC not trust those Tasks any longer (at that given time). This should be sufficient to make users upgrade.

Copy link
Member Author

Choose a reason for hiding this comment

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

Interesting. We should probably mark all tasks which produce or consume the BASE_IMAGE_DIGESTS as deprecated together. I think that should be done outside of the current PR.

task-generator/remote/main.go Show resolved Hide resolved
Comment on lines +124 to +127
if !strings.HasPrefix(ret, "#!") {
// If there is a shebang, it is explicitly non-bash, so don't adjust the image
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another often used option is #!/bin/sh

Copy link
Member Author

Choose a reason for hiding this comment

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

If it is not bash, we cannot inject the remote image adjustment. We cannot design for all potential cases that we might encounter so we just bail out and then can address the cases in the future.

@arewm arewm force-pushed the add-arch-to-multi-platform-images branch from 39ee314 to 50b3e1b Compare August 7, 2024 13:18
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch from 50b3e1b to d36378f Compare August 7, 2024 13:21
In order to reduce the likelihood of users accidentally forgetting to
specify unique tags for each architecture, we can add a suffix to the
pushed image if an arch-specific one doesn't exist.

Signed-off-by: arewm <[email protected]>
@arewm arewm force-pushed the add-arch-to-multi-platform-images branch from d36378f to 02e1a9c Compare August 8, 2024 15:17
@arewm arewm added this pull request to the merge queue Aug 12, 2024
Merged via the queue into konflux-ci:main with commit 420bc92 Aug 12, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants