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

Onbuild to use kaniko instead of docker #1366

Merged
merged 11 commits into from
Nov 6, 2019

Conversation

FelGel
Copy link
Contributor

@FelGel FelGel commented Oct 10, 2019

No description provided.

@@ -647,6 +648,14 @@ func (p *Platform) BuildAndPushContainerImage(buildOptions *containerimagebuilde
return p.containerBuilder.BuildAndPushContainerImage(buildOptions, p.ResolveDefaultNamespace(""))
}

func (p *Platform) GetOnbuildStages(onbuildArtifacts []runtime.Artifact) ([]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not put this in abstract? It's the same behavior for both

artifactIndex := 0

stagedArtifactPaths := make(map[string]string)
for _, artifact := range onbuildArtifacts {
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use artifactIndex as returned from range?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of the artifacts may actually be external images, so a dedicated index counter for onbuild stages is needed

@@ -88,6 +89,49 @@ func (k *Kaniko) BuildAndPushContainerImage(buildOptions *BuildOptions, namespac
return errors.New("Kaniko job has timed out")
}

func (k *Kaniko) GetOnbuildStages(onbuildArtifacts []runtime.Artifact) ([]string, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't it be nicer to returned named stages? That way you don't have to do the whole artifact index thing and it'll be easier to read

@@ -146,7 +146,7 @@ func getContainerBuilderConfiguration(platformConfiguration interface{}) *contai
}
if containerBuilderConfiguration.KanikoImage == "" {
containerBuilderConfiguration.KanikoImage = getEnvOrDefault("NUCLIO_KANIKO_CONTAINER_IMAGE",
"gcr.io/kaniko-project/executor:v0.9.0")
"gcr.io/kaniko-project/executor:v0.13.0")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't forget to add this to artifact version manifest so it will be packaged

@FelGel FelGel changed the base branch from master to development November 4, 2019 14:18
artifact.Name = fmt.Sprintf("onbuildStage-%d", stage)
}

onbuildDockerfileContents := fmt.Sprintf(`FROM %s AS %s
Copy link
Contributor

Choose a reason for hiding this comment

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

Weird formatting?

@@ -876,7 +944,7 @@ func (b *Builder) buildProcessorImage() (string, error) {
return "", errors.Wrap(err, "Failed to get build args")
}

processorDockerfilePathInStaging, err := b.createProcessorDockerfile()
processorDockerfileInfo, err := b.createProcessorDockerfile(b.options.FunctionConfig.Spec.Build.Registry)
Copy link
Contributor

@pavius pavius Nov 5, 2019

Choose a reason for hiding this comment

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

I would make a new variable for this - something like BaseImageRegistry or something. It may very well be different than the registry here (which is used to store the build artifacts)

@pavius pavius merged commit 515cbfa into nuclio:development Nov 6, 2019
@FelGel FelGel deleted the multistage_kaniko branch January 19, 2020 13:39
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.

2 participants