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(CLI): Cannot set Kaniko cache option #1166

Merged
merged 1 commit into from
Dec 19, 2019
Merged

fix(CLI): Cannot set Kaniko cache option #1166

merged 1 commit into from
Dec 19, 2019

Conversation

astefanutti
Copy link
Member

Release Note

NONE

@@ -280,7 +280,8 @@ func (o *installCmdOptions) install(_ *cobra.Command, _ []string) error {
}
}

if !o.KanikoBuildCache {
kanikoBuildCacheFlag := cobraCmd.Flags().Lookup("kaniko-build-cache")
if kanikoBuildCacheFlag.Changed {
Copy link
Contributor

@lburgazzoli lburgazzoli Dec 19, 2019

Choose a reason for hiding this comment

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

why is this needed ? the KanikoBuildCache should be set already by the pre run task and should be true by default

Copy link
Member Author

@astefanutti astefanutti Dec 19, 2019

Choose a reason for hiding this comment

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

With #1121, the default is now false. With falsy default, we have the possibility to set the option even if it's not explicitly provided. Still, it seems more user-friendly not to fill the Spec struct with default values. Boolean is a special case as zero value is also a valid value.

Copy link
Contributor

@lburgazzoli lburgazzoli Dec 19, 2019

Choose a reason for hiding this comment

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

So we can just flip the if previously in place I guess

Copy link
Member Author

@astefanutti astefanutti Dec 19, 2019

Choose a reason for hiding this comment

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

Yes, unless we find it more user-friendly not to fill the Spec struct with default values altogether.

Copy link
Contributor

Choose a reason for hiding this comment

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

but if you flip the switch, the default won't be set, isn't it ?

Copy link
Member Author

@astefanutti astefanutti Dec 19, 2019

Choose a reason for hiding this comment

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

Yes, but if the user explicitly set the option to false, it won't be set either, which doesn't really make sense.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, understood.
so let merge it, problem is that this wont' work with the env + file thing.

I'll think about a solution later on

@lburgazzoli lburgazzoli merged commit 92b635c into apache:master Dec 19, 2019
@astefanutti astefanutti deleted the pr-89 branch December 19, 2019 17:03
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.

None yet

2 participants