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

fn sink and source reference docs #2013

Merged
merged 5 commits into from
May 19, 2021
Merged

Conversation

droot
Copy link
Contributor

@droot droot commented May 14, 2021

This PR does the following:

  • Add reference (help) docs for kpt fn source and kpt fn sink
  • Drops wrap-kind wrap-apiversion flags from kpt fn source. I don't think they are useful. We can introduce them when we see the need. Locking the API surface area.
  • Renamed function-config flag in fn source to fn-config to make it consistent with fn eval

site/reference/fn/sink/README.md Outdated Show resolved Hide resolved
site/reference/fn/source/README.md Outdated Show resolved Hide resolved
kpt fn sink DIR [flags]

DIR:
Path to a local directory to resources to. Directory must exist.
Copy link
Contributor

Choose a reason for hiding this comment

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

Path to a local directory to resources to.

This doesn't read well.

Is DIR required? If so, this behavior is different

Copy link
Contributor Author

Choose a reason for hiding this comment

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

missed write :) fixed.

Yes, DIR is required (probably was done as part of CLI usability work).

if so, this behavior is different

what behavior are you referring to ? and what is different ?

Copy link
Contributor

Choose a reason for hiding this comment

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

DIR was optional before. If unspecified, it prints to stdout.

-->

Implements a [sink function] by reading STDIN and writing configuration.
`sink` reads resources from `stdin` and writes them to a local directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

If there is no directory specified, sink will output to stdout. Will we change this in v1?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

DIR is required argument now. This is changed in v1. Not sure output to stdout makes any sense. If that is the intention, output from source or eval should be good enough, no need of sink. Right ?

Implements a [source function] by reading configuration and writing to STDOUT.
`source` reads resources from a local directory and writes them in `functionSpec`
wire format to `stdout`. The output of the `source` can be pipe'd to commands
such as `kpt fn eval` that accepts `functionSpec` format. It is useful for
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe use the link to function spec here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

-->

Implements a [sink function] by reading STDIN and writing configuration.
`sink` reads resources from `stdin` and writes them to a local directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

The format of stdin for sink and stdout for source are currently underspecified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added brief note about the input for sink and output for source.

-->

Implements a [source function] by reading configuration and writing to STDOUT.
`source` reads resources from a local directory and writes them in `functionSpec`
Copy link
Contributor

Choose a reason for hiding this comment

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

Compare:

kpt fn source wordpress \
  | less

This is wrapped in ResourceList.

kpt fn source wordpress \
  | kpt fn eval --image gcr.io/kpt-fn/kubeval:v0.1 - \
  | less

This is multi-object YAML.

Is there a reason, all wire format as passed through pipes should not be the same:

kpt fn source wordpress \       
  | kpt fn eval --image gcr.io/kpt-fn/set-namespace:v0.1 - -- namespace=mywordpress \
  | kpt fn sink wordpress2

Need to also improve testing for ths.

Copy link
Contributor

@frankfarzan frankfarzan May 14, 2021

Choose a reason for hiding this comment

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

Related to whether we want flexibility of both wrapped and unwrapped with flags:

  1. Source can emit output wrapped or unwrapped
  2. Eval can accept input as wrapped or unwrapped
  3. Eval can emit output as wrapped or unwrapped
  4. Sink can accept input as wrapped or unwrapped

We should think of reasons why one is better over the other. Starting with:

  1. Unwrapped is probably easier to operate on with other tooling (e.g. sed or yq)?
  2. Wrapped is better if we want to include other things in ResourceList (e.g. one of the OpenAPI options @natasha41575 has prototyped)
  3. ?

Copy link
Contributor

@natasha41575 natasha41575 May 14, 2021

Choose a reason for hiding this comment

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

For the OpenAPI scenario specifically: if we choose not to wrap the output in ResourceList (e.g. ResourceList.OpenAPI), another option we'd discussed was wrapping the OpenAPI schema in its own KRM resource (that could be put in ResourceList.items).

I think either unwrapped or wrapped is fine, but I agree that it makes sense to have consistency across commands.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason, all wire format as passed through pipes should not be the same:

I am not familiar with the reasoning. But when it comes to reading resources from a byte stream such as 'stdin, all these command use same kio.ByteReaderthat supports resource in both (wrapped/unwrapped) format without specifying the format, soevalandsinksupports both the formats. And when writing the output, they usekio.ByteWriterthat writes in multi-yaml object format by default unless specified to wrap. Forfn source`, it always write the output in wrapped format.

So looking at the use case of creating your own pipeline using source, eval, sink, source needs to support ResourceList output format. eval needs to support ResourceList input as default (that is does) and it SHOULD output ResourceList by default (and that's where the confusion arises). and sink accepts ResourceList as input.
So only change of default output of eval to ResourceList will bring consistency and will bring it closer to the pipeline execution.

Now output to multi-yaml object has its benefits of being able to pipe to kubectl etc., so that can be supported with a flag.

# output to DIR directory.
$ kpt fn source DIR |
kpt fn eval --image gcr.io/example.com/my-fn - |
kpt fn sink DIR
Copy link
Contributor

@natasha41575 natasha41575 May 14, 2021

Choose a reason for hiding this comment

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

All of the examples here with fn eval also use source and sink together. Do they always have to be used in conjunction? What happens if one is used without the other, e.g. kpt fn source DIR | kpt fn eval, or kpt fn eval | kpt fn sink? It might be helpful to explicitly answer these questions in the documentation or have examples

Copy link
Contributor Author

Choose a reason for hiding this comment

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

source, 'eval' and sink can be used to create your own little pipeline where source acts as a generator (builtin) function, followed by set of eval functions and then final sink function that writes the output. So far, that is the only use-case I know of. I want to see more use-cases of these in the field.

-->

Implements a [source function] by reading configuration and writing to STDOUT.
`source` reads resources from a local directory and writes them in `functionSpec`
Copy link
Contributor

@natasha41575 natasha41575 May 14, 2021

Choose a reason for hiding this comment

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

For the OpenAPI scenario specifically: if we choose not to wrap the output in ResourceList (e.g. ResourceList.OpenAPI), another option we'd discussed was wrapping the OpenAPI schema in its own KRM resource (that could be put in ResourceList.items).

I think either unwrapped or wrapped is fine, but I agree that it makes sense to have consistency across commands.

Copy link
Contributor Author

@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.

Addressed the feedback. I am refraining from making behavioral changes to source, sink or eval as part of this PR. I have updated the reference to document current behavior. Can follow up with another PR for behavior change.

-->

Implements a [sink function] by reading STDIN and writing configuration.
`sink` reads resources from `stdin` and writes them to a local directory.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

DIR is required argument now. This is changed in v1. Not sure output to stdout makes any sense. If that is the intention, output from source or eval should be good enough, no need of sink. Right ?

-->

Implements a [sink function] by reading STDIN and writing configuration.
`sink` reads resources from `stdin` and writes them to a local directory.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added brief note about the input for sink and output for source.

# output to DIR directory.
$ kpt fn source DIR |
kpt fn eval --image gcr.io/example.com/my-fn - |
kpt fn sink DIR
Copy link
Contributor Author

Choose a reason for hiding this comment

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

source, 'eval' and sink can be used to create your own little pipeline where source acts as a generator (builtin) function, followed by set of eval functions and then final sink function that writes the output. So far, that is the only use-case I know of. I want to see more use-cases of these in the field.

Implements a [source function] by reading configuration and writing to STDOUT.
`source` reads resources from a local directory and writes them in `functionSpec`
wire format to `stdout`. The output of the `source` can be pipe'd to commands
such as `kpt fn eval` that accepts `functionSpec` format. It is useful for
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

-->

Implements a [source function] by reading configuration and writing to STDOUT.
`source` reads resources from a local directory and writes them in `functionSpec`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason, all wire format as passed through pipes should not be the same:

I am not familiar with the reasoning. But when it comes to reading resources from a byte stream such as 'stdin, all these command use same kio.ByteReaderthat supports resource in both (wrapped/unwrapped) format without specifying the format, soevalandsinksupports both the formats. And when writing the output, they usekio.ByteWriterthat writes in multi-yaml object format by default unless specified to wrap. Forfn source`, it always write the output in wrapped format.

So looking at the use case of creating your own pipeline using source, eval, sink, source needs to support ResourceList output format. eval needs to support ResourceList input as default (that is does) and it SHOULD output ResourceList by default (and that's where the confusion arises). and sink accepts ResourceList as input.
So only change of default output of eval to ResourceList will bring consistency and will bring it closer to the pipeline execution.

Now output to multi-yaml object has its benefits of being able to pipe to kubectl etc., so that can be supported with a flag.

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.

some nits


1. Multi object YAML where resources are separated by `---`.

2. [Function Specification] wire format where resources are wrapped in an object
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you're linking twice

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Removed the linking here because links are not being rendered in this section (may be because it's a code block)

```

#### Flags

```shell
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: other commands don't use shell directive, so there's little bit of inconsistency in rendering

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for point it out. Cleaned it up.

@droot droot force-pushed the fn-source-sink-reference branch from 4ae301f to ff7b290 Compare May 19, 2021 19:35
@droot droot merged commit 54741cf into kptdev:next May 19, 2021
frankfarzan pushed a commit to frankfarzan/kpt that referenced this pull request Jun 3, 2021
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.

None yet

5 participants