Default to gcr.io/kpt-fn/ #2126

phanimarupaka · 2021-05-27T21:46:05Z

Add default image prefix(gcr.io/kpt-fn/) if full path is not specified.
Introduce short form for --include-meta-resources, m flag.

frankfarzan · 2021-05-27T21:47:59Z

e2e tests? docs?

e2e/testdata/fn-eval/simple-function/.expected/config.yaml

mikebz · 2021-05-29T00:48:56Z

I like this change a lot. It will help us with adoption of the pipeline in Skaffold. They are looking for simplicity @yuwenma .

phanimarupaka · 2021-05-31T04:54:26Z

e2e tests? docs?

Already added one e2e test and updated unit tests. Added reference docs.

thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go

site/reference/fn/eval/README.md

thirdparty/cmdconfig/commands/cmdeval/cmdeval.go

droot · 2021-06-02T17:21:32Z

@phanimarupaka I don't see the change implemented for fn render. Is that intentional ?

We can make this change in a central place in fnruntime/runner so that it is consistently applied to both eval and render.

droot

The change is looking good, but needs a rebase.

phanimarupaka · 2021-06-09T20:34:14Z

The change is looking good, but needs a rebase.

@droot Done.

site/reference/cli/fn/eval/README.md

site/reference/cli/fn/doc/README.md

site/reference/cli/fn/eval/README.md

e2e/testdata/fn-render/basicpipeline/Kptfile

package-examples/pipeline-validate/README.md

site/book/01-getting-started/02-quickstart.md

site/book/04-using-functions/01-declarative-function-execution.md

e2e/testdata/fn-eval/simple-function-short-image-path/.expected/config.yaml

e2e/testdata/fn-render/basicpipeline-image-short-path/.expected/diff.patch

droot · 2021-06-14T20:29:14Z

internal/fnruntime/container.go

@@ -197,6 +197,7 @@ func (f *ContainerFn) prepareImage() error {
 	// This can help to ensure we have the latest release for "moving tags" like
 	// v1 and v1.2. The performance cost is very minimal, since `docker pull`
 	// checks the SHA first and only pull the missing docker layer(s).
+	f.Image = AddDefaultImagePathPrefix(f.Image)


We probably want to resolve this at the higher layer. Look where we create containerRunner and functionRunner in runner.go because there might be other pieces where full name is probably desired ?

That is not propagating it in all places. It is safe to resolve it as places we make calls to docker cli and displaying results.

I looked at the code. There are two places where the changes will ensure that they are propagated in all places:

for fn eval, we should resolve this in the command runner itself like we are doing in the cmdfndoc.go

for fn render, doing it in fnChain function which will ensure it's propagated in all places. Another option is doing it in pkg.Pipeline() function.

Calling it individually where we make calls to docker or displayResults is not safe IMO.

package-examples/pipeline-validate/README.md

site/book/04-using-functions/02-imperative-function-execution.md

droot

I have a suggestion on where to resolve the image in the call stack. Happy to discuss.

site/book/04-using-functions/02-imperative-function-execution.md

site/reference/cli/fn/doc/README.md

e2e/testdata/fn-render/short-image-path/.expected/results.yaml

droot · 2021-07-07T22:10:27Z

internal/fnruntime/container.go

@@ -197,6 +197,7 @@ func (f *ContainerFn) prepareImage() error {
 	// This can help to ensure we have the latest release for "moving tags" like
 	// v1 and v1.2. The performance cost is very minimal, since `docker pull`
 	// checks the SHA first and only pull the missing docker layer(s).
+	f.Image = AddDefaultImagePathPrefix(f.Image)


I looked at the code. There are two places where the changes will ensure that they are propagated in all places:

for fn eval, we should resolve this in the command runner itself like we are doing in the cmdfndoc.go

for fn render, doing it in fnChain function which will ensure it's propagated in all places. Another option is doing it in pkg.Pipeline() function.

Calling it individually where we make calls to docker or displayResults is not safe IMO.

internal/fnruntime/runner.go

internal/fnruntime/utils.go

site/book/04-using-functions/01-declarative-function-execution.md

internal/fnruntime/container.go

mikebz reviewed May 29, 2021

View reviewed changes

e2e/testdata/fn-eval/simple-function/.expected/config.yaml Outdated Show resolved Hide resolved

phanimarupaka force-pushed the DefaultToGcrKptfn branch from 85f514d to a0f83f3 Compare May 31, 2021 04:52

phanimarupaka requested a review from frankfarzan as a code owner May 31, 2021 04:52

phanimarupaka requested a review from mikebz May 31, 2021 04:54

frankfarzan reviewed Jun 1, 2021

View reviewed changes

thirdparty/cmdconfig/commands/cmdeval/cmdeval_test.go Outdated Show resolved Hide resolved

site/reference/fn/eval/README.md Outdated Show resolved Hide resolved

thirdparty/cmdconfig/commands/cmdeval/cmdeval.go Outdated Show resolved Hide resolved

frankfarzan requested a review from droot June 1, 2021 22:29

phanimarupaka force-pushed the DefaultToGcrKptfn branch 2 times, most recently from 3775220 to e7ee118 Compare June 6, 2021 01:39

phanimarupaka requested review from mengqiy and frankfarzan June 6, 2021 01:58

droot requested changes Jun 9, 2021

View reviewed changes

phanimarupaka force-pushed the DefaultToGcrKptfn branch from e7ee118 to a62ce79 Compare June 9, 2021 20:33

droot reviewed Jun 9, 2021

View reviewed changes

site/reference/cli/fn/eval/README.md Show resolved Hide resolved

phanimarupaka force-pushed the DefaultToGcrKptfn branch from a62ce79 to db69c16 Compare June 10, 2021 17:52

droot approved these changes Jun 10, 2021

View reviewed changes

frankfarzan suggested changes Jun 10, 2021

View reviewed changes

site/reference/cli/fn/doc/README.md Show resolved Hide resolved

site/reference/cli/fn/eval/README.md Show resolved Hide resolved

e2e/testdata/fn-render/basicpipeline/Kptfile Outdated Show resolved Hide resolved

phanimarupaka force-pushed the DefaultToGcrKptfn branch from db69c16 to 52d2487 Compare June 14, 2021 18:49

frankfarzan reviewed Jun 14, 2021

View reviewed changes

package-examples/pipeline-validate/README.md Outdated Show resolved Hide resolved

site/book/01-getting-started/02-quickstart.md Outdated Show resolved Hide resolved

site/book/04-using-functions/01-declarative-function-execution.md Outdated Show resolved Hide resolved

droot requested changes Jun 14, 2021

View reviewed changes

phanimarupaka added 6 commits July 7, 2021 10:54

Default to gcr.io/kpt-fn/

985b63e

reference docs

7d98f70

Update tests

b8c2d6f

Extend to render and fndoc

fae3f40

Add new tests

ca1c3c8

Update book and package examples docs

05df457

phanimarupaka added 6 commits July 7, 2021 11:17

Delete helloworld examples

cac8522

Rebase updates

fab9257

Rename examples

a71af6c

Fully resolved path for results display

75267b5

Update render docs

af2452b

Update e2e tests for eval and render

2b9fd04

phanimarupaka force-pushed the DefaultToGcrKptfn branch from 52d2487 to 2b9fd04 Compare July 7, 2021 19:01

phanimarupaka added 2 commits July 7, 2021 12:18

Move the call up in stack

a2dffa9

Short hand for --include-meta-resources

3bcde50

phanimarupaka force-pushed the DefaultToGcrKptfn branch from 7f017c9 to 3bcde50 Compare July 7, 2021 19:24

Move call to container.go

37cce56

phanimarupaka requested review from frankfarzan and droot July 7, 2021 20:40

frankfarzan suggested changes Jul 7, 2021

View reviewed changes

site/book/04-using-functions/02-imperative-function-execution.md Show resolved Hide resolved

droot requested changes Jul 7, 2021

View reviewed changes

phanimarupaka added 2 commits July 7, 2021 16:38

Suggested changes

4356ada

generate docs

148c67e

phanimarupaka requested review from droot and frankfarzan July 7, 2021 23:40

droot approved these changes Jul 7, 2021

View reviewed changes

Update eval tests

cfa9ad7

frankfarzan suggested changes Jul 8, 2021

View reviewed changes

site/book/04-using-functions/01-declarative-function-execution.md Outdated Show resolved Hide resolved

internal/fnruntime/container.go Show resolved Hide resolved

Changes

ed2c92d

phanimarupaka requested a review from frankfarzan July 8, 2021 18:48

frankfarzan approved these changes Jul 8, 2021

View reviewed changes

phanimarupaka merged commit 06adff5 into kptdev:main Jul 8, 2021

phanimarupaka linked an issue Jul 15, 2021 that may be closed by this pull request

Prepend gcr.io/kpt-fn/ prefix by default to image and short hand for include-meta-resources flag #2401

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default to gcr.io/kpt-fn/ #2126

Default to gcr.io/kpt-fn/ #2126

phanimarupaka commented May 27, 2021 •

edited

frankfarzan commented May 27, 2021

mikebz commented May 29, 2021

phanimarupaka commented May 31, 2021 •

edited

droot commented Jun 2, 2021

droot left a comment

phanimarupaka commented Jun 9, 2021

droot Jun 14, 2021

phanimarupaka Jul 7, 2021 •

edited

droot Jul 7, 2021

droot left a comment

droot Jul 7, 2021

Default to gcr.io/kpt-fn/ #2126

Default to gcr.io/kpt-fn/ #2126

Conversation

phanimarupaka commented May 27, 2021 • edited

frankfarzan commented May 27, 2021

mikebz commented May 29, 2021

phanimarupaka commented May 31, 2021 • edited

droot commented Jun 2, 2021

droot left a comment

Choose a reason for hiding this comment

phanimarupaka commented Jun 9, 2021

droot Jun 14, 2021

Choose a reason for hiding this comment

phanimarupaka Jul 7, 2021 • edited

Choose a reason for hiding this comment

droot Jul 7, 2021

Choose a reason for hiding this comment

droot left a comment

Choose a reason for hiding this comment

droot Jul 7, 2021

Choose a reason for hiding this comment

phanimarupaka commented May 27, 2021 •

edited

phanimarupaka commented May 31, 2021 •

edited

phanimarupaka Jul 7, 2021 •

edited