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

use cgr.dev for base img make the const accessible #904

Conversation

developer-guy
Copy link
Collaborator

Signed-off-by: Batuhan Apaydın [email protected]

According to the comment here

@developer-guy developer-guy force-pushed the feature/change-default-base-img branch 4 times, most recently from 943907a to 63b2f4e Compare December 15, 2022 08:45
@developer-guy developer-guy force-pushed the feature/change-default-base-img branch 2 times, most recently from 489ba6a to b23ec59 Compare December 23, 2022 08:51
@developer-guy developer-guy force-pushed the feature/change-default-base-img branch 3 times, most recently from da1e840 to c228eef Compare December 24, 2022 08:41
@developer-guy
Copy link
Collaborator Author

This PR will fix the problem created by #910. Can you please check @halvards? PTAL @imjasonh

pkg/build/gobuild.go Outdated Show resolved Hide resolved
pkg/build/gobuild.go Outdated Show resolved Hide resolved
pkg/build/gobuild.go Outdated Show resolved Hide resolved
@developer-guy developer-guy force-pushed the feature/change-default-base-img branch from c228eef to 0e0fc16 Compare January 4, 2023 15:56
@developer-guy developer-guy requested review from imjasonh and removed request for imjasonh January 4, 2023 15:56
// DefaultBaseImage returns the default base image to use
func DefaultBaseImage() (ref string, err error) {
if !viper.IsSet("defaultBaseImage") {
return "", errors.New("'defaultBaseImage' is not set")
Copy link
Member

Choose a reason for hiding this comment

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

Thinking about this some more, I think we might want to add a call to SetDefault somewhere in here. If some external code calls DefaultBaseImage without previously calling LoadConfig, then defaultBaseImage will be unset, and the user shouldn't get an error; they should get the const default.

This logic also applies to having this code consider KO_CONFIG_PATH when it's called; if someone calls this without first calling LoadConfig, they won't get the value configured in their config file, they'll get an error.

Comment on lines +146 to +150
// If the base image is set in the config, use that.
if baseImage := v.GetString("defaultBaseImage"); baseImage != configDefaultBaseImage {
// to ensure to set the base image from the config file if it differs from the default
viper.SetDefault("defaultBaseImage", baseImage)
}
Copy link
Member

Choose a reason for hiding this comment

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

Does this have any effect? This seems to be saying, "if the value of defaultBaseImage is not the const default, then set the default value of defaultBaseImage to that value", but that's where we got the value from in the first place.

@developer-guy
Copy link
Collaborator Author

This PR will fix the problem created by #910. Can you please check @halvards? PTAL @imjasonh

Quick update here, @imjasonh fixed the problem in PR #923, so I've updated my PR to revert changes related to that fix. ☝️

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

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.

2 participants