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

Out of place hydration using render and eval #2137

Merged
merged 12 commits into from
Jun 5, 2021

Conversation

phanimarupaka
Copy link
Contributor

@phanimarupaka phanimarupaka commented May 30, 2021

Commit 1:

  1. Add --output(-o) flag to render and eval commands, to support out of place hydration and writes rendered resources to stdout(stdout wrapped)|unwrap(stdout unwrapped)|OUT_DIR_PATH(to another directory). (kpt fn render should support out-of-place sink-mode #1412)
  2. Deprecate --dry-run flag from eval command as it is supported by --output flag.

Commit 2
Add e2e tests for oop hydration

Commit 3
Reference docs.

Supported example

kpt fn render [HERE] -o stdout \
| kpt fn eval --image gcr.io/kpt-fn/set-annotations:v0.1.3 -- foo=bar \
| kpt fn eval --image gcr.io/kpt-fn/set-labels:v0.1.3 -o [stdout|unwrap|OUT_DIR]-- tier=backend

Note: render can only read from directory and not STDIN. eval can read from both stdin and directory.

@phanimarupaka phanimarupaka changed the title Out of place render Out of place hydration using render and eval May 30, 2021
@phanimarupaka phanimarupaka requested a review from mikebz May 30, 2021 22:01
@phanimarupaka phanimarupaka force-pushed the OutOfPlaceRender branch 2 times, most recently from ba3b3ff to 13f63d5 Compare May 31, 2021 04:19
@phanimarupaka phanimarupaka linked an issue May 31, 2021 that may be closed by this pull request
@frankfarzan
Copy link
Contributor

frankfarzan commented Jun 1, 2021

This PR is doing too many things, and combines multiple UX changes each deserving their own consideration e.g (Using dry-run in render for output) and e2e testing. Let's split this up into multiple orthogonal PRs.

@phanimarupaka phanimarupaka marked this pull request as draft June 1, 2021 17:28
@morgante
Copy link
Contributor

morgante commented Jun 2, 2021

@phanimarupaka Thanks for working on this! 💯 From a cursory look it seems this would indeed unblock the hydration functionality needed for LZ.

@phanimarupaka phanimarupaka force-pushed the OutOfPlaceRender branch 3 times, most recently from e4ec538 to 5cff579 Compare June 3, 2021 18:27
@phanimarupaka phanimarupaka marked this pull request as ready for review June 3, 2021 18:32
Copy link
Contributor

@frankfarzan frankfarzan left a comment

Choose a reason for hiding this comment

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

Please add the different usages to the reference docs and make sure they have e2e tests. To make this manageable consider splitting into separate PRs, one per command.

Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Did a quick pass of the Go code only.

internal/cmdrender/cmd.go Outdated Show resolved Hide resolved
internal/cmdrender/cmd.go Outdated Show resolved Hide resolved
internal/cmdrender/executor.go Outdated Show resolved Hide resolved
internal/cmdrender/executor.go Show resolved Hide resolved
internal/util/cmdutil/cmdutil.go Show resolved Hide resolved
internal/util/cmdutil/cmdutil.go Show resolved Hide resolved
internal/util/cmdutil/cmdutil.go Outdated Show resolved Hide resolved
thirdparty/cmdconfig/commands/cmdeval/cmdeval.go Outdated Show resolved Hide resolved
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Second pass only on the Go code, will review the docs/tests in the morning.

internal/cmdrender/executor.go Outdated Show resolved Hide resolved
internal/cmdrender/executor.go Outdated Show resolved Hide resolved
internal/cmdrender/executor.go Show resolved Hide resolved
internal/cmdrender/executor.go Show resolved Hide resolved
internal/cmdrender/executor.go Outdated Show resolved Hide resolved
internal/util/cmdutil/cmdutil.go Show resolved Hide resolved
internal/util/cmdutil/cmdutil.go Outdated Show resolved Hide resolved
internal/util/cmdutil/cmdutil.go Show resolved Hide resolved
e2e/testdata/fn-eval/out-of-place-dir/.expected/exec.sh Outdated Show resolved Hide resolved
e2e/testdata/fn-eval/out-of-place-dir/.expected/exec.sh Outdated Show resolved Hide resolved
site/reference/fn/eval/README.md Outdated Show resolved Hide resolved
site/reference/fn/eval/README.md Outdated Show resolved Hide resolved
site/reference/fn/eval/README.md Outdated Show resolved Hide resolved
site/reference/fn/render/README.md Outdated Show resolved Hide resolved
thirdparty/cmdconfig/commands/cmdeval/cmdeval.go Outdated Show resolved Hide resolved
internal/cmdrender/executor.go Show resolved Hide resolved
internal/util/cmdutil/cmdutil.go Outdated Show resolved Hide resolved
internal/util/cmdutil/cmdutil.go Outdated Show resolved Hide resolved
Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Looking in good shape, few more comments.

e2e/testdata/fn-eval/out-of-place-dir/.expected/exec.sh Outdated Show resolved Hide resolved
e2e/testdata/fn-render/out-of-place-dir/.expected/exec.sh Outdated Show resolved Hide resolved
internal/cmdrender/executor.go Outdated Show resolved Hide resolved
internal/cmdrender/executor.go Show resolved Hide resolved
thirdparty/kyaml/runfn/runfn.go Show resolved Hide resolved
@frankfarzan
Copy link
Contributor

/lgtm with the suggested follow up on non-structured results. Please wait for @droot to approve.

Copy link
Contributor

@droot droot left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. This is good to go 🚢

We need to follow up with:

  1. We need to update the go/kpt-testcases sheet for out-of-place
  2. non-structructured output to be send to stderr always: that will remove conditional printing
  3. Removing Stdin (-) from fn eval

@phanimarupaka phanimarupaka merged commit 7d765c3 into kptdev:next Jun 5, 2021
@frankfarzan
Copy link
Contributor

  1. Source using -output consistently.
  2. Outputting meta resources (cross-cutting).

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.

kpt fn render should support out-of-place sink-mode
4 participants