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
Refactor KanikoBuild into KanikoArtifact and Cluster #1797
Refactor KanikoBuild into KanikoArtifact and Cluster #1797
Conversation
so that other artifacts can use the function.
This commit removes the KanikoBuild type, which is confusing as it reprseents both a builder and an environment. It also assumes that certain elements (buildcontext) apply to all artifacts, when in reality different artifacts should have different buildcontext. KanikoBuild is replaced with a KanikoArtifact type, which can be specified per artifact. I also added a ClusterDetails field to contain info about the cluster itself (namespace, timeouts, secrets etc)
pkg/skaffold/build/kaniko/run.go
Outdated
args = append(args, docker.GetBuildArgs(artifact.DockerArtifact)...) | ||
|
||
// TODO: remove since AdditionalFlags will be deprecated (priyawadhwa@) | ||
args = append(args, kanikoArtifact.AdditionalFlags...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's put out a deprecation warning here into the log if AdditionalFlags
are not empty?
if err := withClusterConfig(c, | ||
setDefaultClusterNamespace, | ||
setDefaultClusterTimeout, | ||
setDefaultClusterSecret, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
setDefaultClusterSecret, | |
setDefaultClusterPullSecret, |
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with some nits
Codecov Report
@@ Coverage Diff @@
## master #1797 +/- ##
=========================================
+ Coverage 45.48% 45.7% +0.21%
=========================================
Files 142 142
Lines 6617 6671 +54
=========================================
+ Hits 3010 3049 +39
- Misses 3306 3316 +10
- Partials 301 306 +5
Continue to review full report at Codecov.
|
1 similar comment
Codecov Report
@@ Coverage Diff @@
## master #1797 +/- ##
=========================================
+ Coverage 45.48% 45.7% +0.21%
=========================================
Files 142 142
Lines 6617 6671 +54
=========================================
+ Hits 3010 3049 +39
- Misses 3306 3316 +10
- Partials 301 306 +5
Continue to review full report at Codecov.
|
@priyawadhwa you might want to rebase on top of master instead, you'll get improvements to the integration tests |
Thanks for the tip @dgageot ! |
This PR refactors kaniko, taking the preexisting KanikoBuild type and splitting it up into a per artifact KanikoArtifact and Cluster type.
KanikoArtifact
now contains artifact specified fields (buildcontext, cache, additional flags etc.), whileClusterDetails
contains fields specific to the cluster (namespace, timeout, secrets). This makes more sense to me, since a buildcontext should be different for different artifacts (different bucket to pull from, etc).This PR updates the config to
v1beta8
as it represents a breaking change to the skaffold config.I also added an integration test to make sure kaniko can build multiple artifacts.
This refactor will ultimately make it much easier to implement #1549