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

fix(build): complete repo name for default baseImage #4480

Merged
merged 2 commits into from
Jun 22, 2023

Conversation

gansheer
Copy link
Contributor

@gansheer gansheer commented Jun 12, 2023

Motivation

The baseImage is repository partially complete. To be correct we need to have the full name or the short name.

Description

Remove the docker.io part into the repository name for the baseImage.
Add some code to manage the Buildah publish strategy's default baseImage that needs a full name. In case of override of the default baseImage the user is in charge of ensuring it is valid.

Release Note

fix: Shorten baseImage declaration to eclipse-temurin:11 and manage Buildah publish strategy

Copy link
Contributor

@squakez squakez left a comment

Choose a reason for hiding this comment

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

I am not really sure if that would be the correct way to pull the image from Docker Hub catalog. According to the Docker documentation, the path should be even without the docker.io [1] as it assumes by default that is the registry to use. Is it required by any chance in some kubernetes cluster like OCP?

[1] https://docs.docker.com/engine/reference/commandline/pull/#pull-from-a-different-registry

@gansheer
Copy link
Contributor Author

gansheer commented Jun 13, 2023

I am not really sure if that would be the correct way to pull the image from Docker Hub catalog. According to the Docker documentation, the path should be even without the docker.io [1] as it assumes by default that is the registry to use. Is it required by any chance in some kubernetes cluster like OCP?

[1] https://docs.docker.com/engine/reference/commandline/pull/#pull-from-a-different-registry

The docker.io part was introduced by this PR#3309, so I guess Buildah needs it. I could try to remove the docker.io if we think we have enough tests for Buildah (and multi-architecture).

@squakez
Copy link
Contributor

squakez commented Jun 13, 2023

Yeah, let's try removing the domain name and see if the default is good enough also for other publish strategies. If it does not work, we should instead apply any specific logic to the affected publishing strategy.

@gansheer gansheer force-pushed the fix/baseimage_full_repo_name branch 3 times, most recently from f00fd07 to 594ceeb Compare June 14, 2023 12:17
@gansheer gansheer force-pushed the fix/baseimage_full_repo_name branch 3 times, most recently from 8dc613c to c678e7f Compare June 14, 2023 14:08
@gansheer gansheer force-pushed the fix/baseimage_full_repo_name branch from c678e7f to a75df76 Compare June 20, 2023 13:58
@gansheer gansheer force-pushed the fix/baseimage_full_repo_name branch from a75df76 to 8b153cf Compare June 20, 2023 14:04
@gansheer
Copy link
Contributor Author

gansheer commented Jun 21, 2023

@squakez @oscerd @christophd can one of you trigger the workflows please 😄

@squakez
Copy link
Contributor

squakez commented Jun 22, 2023

I've restarted the failing checks now that the actions are back stable. If all is green, is this good to merge?

@squakez squakez merged commit fc85997 into apache:main Jun 22, 2023
15 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.

3 participants