cdk8s-team / cdk8s

Define Kubernetes native apps and abstractions using object-oriented programming
https://cdk8s.io
Apache License 2.0
4.29k stars 290 forks source link

`cdk8s synth --stdout` doesn't produce valid yaml output #677

Open trondhindenes opened 3 years ago

trondhindenes commented 3 years ago

Description of the bug:

When running cdk8s synth --stdout the output isn't valid yaml. This is because the first line output is a file name. This makes it unnecessarily difficult to perform further parsing of the output, such as yaml-loading it to split it up into separate files etc.

I would expect the --stdout flag to produce valid yaml

Reproduction Steps:

run cdk8s synth --stdout

Error Log:

Environment:

Other:


This is :bug: Bug Report

olivercodes commented 2 years ago

New contributor here. Looking at this issue, I found this line: https://github.com/cdk8s-team/cdk8s-cli/blob/main/src/cli/cmds/synth.ts#L34

Also looks like the tests could use some love https://github.com/cdk8s-team/cdk8s-cli/blob/main/test/synth/synth-stdout.test.ts


Is there a design decision as to why the file path is printed at the top?

Also, a less obvious thing to consider is that if you run cdk8s synth --stdout on a newly initialized project (say you are following the getting started guide) you will just get an empty response (if we remove the path). Not a terrible thing, just thought I'd mention it.


Thoughts:

If there's a valid reason to keep the path, maybe we just need a yaml output flag

trondhindenes commented 2 years ago

a yaml flag is a good idea. Important to make it discoverable when running --help tho.

Chriscbr commented 2 years ago

I think omitting the file path when --stdout is passed or printing it to stderr instead seems reasonable to me.

@olivercodes I also think you have a good point, perhaps including one resource in the starter code (so that the initial output isn't empty) would improve the experience for newcomers.

Contributions are welcome!

olivercodes commented 2 years ago

That's a good point, if the starter just had a single basic resource, the value of checking for an empty resource (and printing something meaningful) becomes less valuable/common.

I'll take this on and submit a PR (2 I guess, 1 to the examples and 1 to the cli)

On Tue, Oct 5, 2021 at 7:14 PM Christopher Rybicki @.***> wrote:

I think omitting the file path when --stdout is passed or printing it to stderr instead seems reasonable to me.

@olivercodes https://github.com/olivercodes I also think you have a good point, perhaps including one resource in the starter code (so that the initial output isn't empty) would improve the experience for newcomers.

Contributions are welcome!

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/cdk8s-team/cdk8s/issues/677#issuecomment-935029894, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABSFZASSDJIE3EBDI56WPJTUFOBEJANCNFSM5EBZNRBQ .

github-actions[bot] commented 2 years ago

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon.

mrliptontea commented 2 years ago

I'd like to make a suggestion - assuming file names in the output are important and intentional, can they not be preceded with # to make a YAML comment? E.g.:

# /tmp/cdk8s-JnaTCh/app.k8s.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
…

I am a bit surprised this isn't being addressed or discussed - I'd thought that cdk8s synth | kubectl apply -f - would be something that a lot of people would use, especially those coming from kustomize which only recently updated its embedded version so previously had to use kustomize build | kubectl apply -f -.

github-actions[bot] commented 2 years ago

This issue is now marked as stale because it hasn't seen activity for a while. Add a comment or it will be closed soon. If you wish to exclude this issue from being marked as stale, add the "backlog" label.

github-actions[bot] commented 1 year ago

This issue has not received any attention in 1 year and will be closed soon. If you want to keep it open, please leave a comment below @mentioning a maintainer.

iliapolo commented 1 year ago

Keep open

diefans commented 7 months ago

I guess the simplest and sanest way of joining yaml files is to have them all have a "yaml document separator" a.k.a. --- in place... the fix would land here: https://github.com/cdk8s-team/cdk8s-core/blob/2.x/src/yaml.ts#L44