Refactor kaniko settings #3032
Refactor kaniko settings #3032
Conversation
There was a problem hiding this comment.
I think we should get rid of IsKanikoCacheEnabled() so we can really have no reference to any implementation detail. If you need to check if cache is enabled, you may just check the new map property.
You should format also running gofmt to have the changes formatted properly
pkg/platform/defaults.go
Outdated
| @@ -169,8 +170,11 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose bool) error { | |||
| "-Dstyle.color=never", | |||
| } | |||
| } | |||
| if p.Status.Build.PersistentVolumeClaim == "" { | |||
| p.Status.Build.PersistentVolumeClaim = p.Name | |||
| //if p.Status.Build.PersistentVolumeClaim == "" { | |||
There was a problem hiding this comment.
We can remove those comments to keep the source code more readable
There was a problem hiding this comment.
ok but I think that now with the map in place we have to have some additional logic to check if the key is present and then parsing the value. In this case IsKanikoCacheEnabled() helps because we don't need to do it everywhere.
There was a problem hiding this comment.
Maybe we can generalize that with IsOptionEnabled(key string) bool? the main goal is to hide any implementation detail from the API definition.
mmelko
force-pushed
the
main
branch
2 times, most recently
from
153f805
to
b889082
Compare
| if b.KanikoBuildCache == nil { | ||
| // Cache is disabled by default | ||
| return false | ||
| // IsOptionEnabled tells if the KanikoCache is enabled on the integration platform build spec |
There was a problem hiding this comment.
Still reference to Kaniko. As it now is a general purpose we should change the comment as well
| @@ -70,7 +70,11 @@ func (action *initializeAction) Handle(ctx context.Context, platform *v1.Integra | |||
| } | |||
|
|
|||
| if platform.Status.Build.PublishStrategy == v1.IntegrationPlatformBuildPublishStrategyKaniko { | |||
There was a problem hiding this comment.
You can comment this block with // nolint: staticcheck to avoid lint complains (due to the deprecation)
| @@ -40,6 +40,11 @@ func createKanikoCacheWarmerPod(ctx context.Context, client client.Client, platf | |||
| // See: | |||
| // - https://kubernetes.io/docs/concepts/storage/persistent-volumes/#node-affinity | |||
| // - https://kubernetes.io/docs/concepts/storage/volumes/#local | |||
| pvcName := platform.Status.Build.PersistentVolumeClaim | |||
pkg/platform/defaults.go
Outdated
| @@ -169,8 +173,8 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose bool) error { | |||
| "-Dstyle.color=never", | |||
| } | |||
| } | |||
| if p.Status.Build.PersistentVolumeClaim == "" { | |||
| p.Status.Build.PersistentVolumeClaim = p.Name | |||
| if _, ok := p.Status.Build.PublishStrategyOptions["persistentVolumeClaim"]; !ok { | |||
There was a problem hiding this comment.
Should not better use builder.KanikoPVCName constant?
| @@ -189,15 +193,15 @@ func setPlatformDefaults(p *v1.IntegrationPlatform, verbose bool) error { | |||
| Duration: 5 * time.Minute, | |||
| } | |||
| } | |||
|
|
|||
| if p.Status.Build.PublishStrategy == v1.IntegrationPlatformBuildPublishStrategyKaniko && p.Status.Build.KanikoBuildCache == nil { | |||
| _, cacheEnabled := p.Status.Build.PublishStrategyOptions["KanikoBuildCache"] | |||
There was a problem hiding this comment.
Should not better use builder.KanikoBuildCacheEnabled constant?
| @@ -113,6 +113,18 @@ func (t *builderTrait) Apply(e *Environment) error { | |||
| }}) | |||
|
|
|||
| case v1.IntegrationPlatformBuildPublishStrategyKaniko: | |||
|
@squakez thanks for your feedback! I updated the PR accordingly |
Refactor kaniko settings by moving existing fields into map[string] field.
fix #3005
Release Note