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.

Sign up for GitHub

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

Jump to bottom

cdk8s synth -p fails to insert necessary document start markers #2155

Open
rassie opened this issue May 4, 2023 · 4 comments · May be fixed by cdk8s-team/cdk8s-cli#1928
Open

cdk8s synth -p fails to insert necessary document start markers #2155

rassie opened this issue May 4, 2023 · 4 comments · May be fixed by cdk8s-team/cdk8s-cli#1928
Labels
bug Something isn't working @component/cdk8s-cli Issue related to cdk8s-cli effort/small 1 day tops priority/p2 Dependent on community feedback. PR's are welcome :)

Comments

@rassie
Copy link

rassie commented May 4, 2023

Description of the bug:

If cdk8s synth -p is used with any other yamlOutputType setting on the app than YamlOutputType.FILE_PER_APP, the YAML being output is missing some document start markers.

Reproduction Steps:

  1. Create a new cdk8s project
  2. Adapt this project as per https://cdk8s.io/docs/latest/getting-started/#importing-constructs-for-the-kubernetes-api
  3. Change const app = new App(); to const app = new App({ yamlOutputType: YamlOutputType.FILE_PER_RESOURCE }); in main.ts
  4. Execute npm run compile && cdk8s synth -p
  5. Observe the following output:
apiVersion: apps/v1
kind: Deployment
metadata:
  name: hello-deployment-c8c7fda7
spec:
  replicas: 2
  selector:
    matchLabels:
      app: hello-k8s
  template:
    metadata:
      labels:
        app: hello-k8s
    spec:
      containers:
        - image: paulbouwer/hello-kubernetes:1.7
          name: hello-kubernetes
          ports:
            - containerPort: 8080
apiVersion: v1
kind: Service
metadata:
  name: hello-service-c8c17160
spec:
  ports:
    - port: 80
      targetPort: 8080
  selector:
    app: hello-k8s
  type: LoadBalancer

As you can see, this output is missing the YAML document start marker --- before the second resource.

Other:

The reason is obviously that synth -p is not outputting documents one by one, but instead it runs synth into a temporary directory and then concatenated the files generated. The document start marker is missing at the exact spot where two files come together. For obvious reasons, this is not a problem with YamlOutputType.FILE_PER_APP.

A simple solution could be adding a document start marker to each file or each resource by default (which is recommended anyway, e.g. in yamllint).

This is 🐛 Bug Report
@rassie rassie added bug Something isn't working needs-triage Priority and effort undetermined yet labels May 4, 2023
@sumupitchayan
Copy link
Contributor

@rassie is there a reason you are running cdk8s synth with the -p flag in conjunction with setting the yaml output type to YamlOutputType.FILE_PER_RESOURCE? As you mentioned, running cdk8s synth -p with the default yaml output type does not cause this problem.

@sumupitchayan sumupitchayan added the response-requested Awaiting response from author label May 18, 2023
@rassie
Copy link
Author

rassie commented May 18, 2023

I'm using cdk8s with Argo-CD, where I rely on -p to output the manifests. However, I do not control every single deployment repository, so it's up to those developers to set the output type correctly and with "per resource" being the default, I would just like to be sure that -p works correctly with every output type.

@github-actions github-actions bot removed the response-requested Awaiting response from author label May 18, 2023
rassie referenced this issue in rassie/cdk8s-cli Feb 8, 2024
@rassie
Add YAML document separator to each manifest
fe7bf39 
When output to STDOUT is requested, each manifest in an app is rendered separately and dumped to standard output.
In case an app contains more than one chart, a document separator will be missing between adjacent charts, since there
is no YAML document separator either at the beginning or at the end of an individual output. The last resource of the
preceding chart and the first resource of the following chart will be merged into a random mess. By adding a YAML
document separator after each manifest unconditionally, this can be cleanly avoided.

Fixes: #943
@rassie rassie linked a pull request Feb 8, 2024 that will close this issue
Open
rassie referenced this issue in rassie/cdk8s-cli Feb 8, 2024
@rassie
Add YAML document separator to each manifest
13a9d61 
When output to STDOUT is requested, each manifest in an app is rendered separately and dumped to standard output.
In case an app contains more than one chart, a document separator will be missing between adjacent charts, since there
is no YAML document separator either at the beginning or at the end of an individual output. The last resource of the
preceding chart and the first resource of the following chart will be merged into a random mess. By adding a YAML
document separator after each manifest unconditionally, this can be cleanly avoided.

Fixes: #943

Signed-off-by: Nikolai Prokoschenko <[email protected]>
rassie referenced this issue in rassie/cdk8s-cli Feb 8, 2024
@rassie
Add YAML document separator to each manifest
5536128 
When output to STDOUT is requested, each manifest in an app is rendered separately and dumped to standard output.
In case an app contains more than one chart, a document separator will be missing between adjacent charts, since there
is no YAML document separator either at the beginning or at the end of an individual output. The last resource of the
preceding chart and the first resource of the following chart will be merged into a random mess. By adding a YAML
document separator after each manifest unconditionally, this can be cleanly avoided.

Fixes: #943

Signed-off-by: Nikolai Prokoschenko <[email protected]>
@github-actions GitHub Actions
Copy link
Contributor

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.

@github-actions github-actions bot added the closing-soon Issue/PR will be closing soon if no response is provided label May 17, 2024
@rassie
Copy link
Author

rassie commented May 17, 2024

@iliapolo should be still valid.

@github-actions github-actions bot removed the closing-soon Issue/PR will be closing soon if no response is provided label May 18, 2024
@iliapolo iliapolo added effort/small 1 day tops priority/p2 Dependent on community feedback. PR's are welcome :) and removed needs-triage Priority and effort undetermined yet labels Jun 1, 2024
@iliapolo iliapolo added the @component/cdk8s-cli Issue related to cdk8s-cli label Sep 20, 2024
@iliapolo iliapolo transferred this issue from cdk8s-team/cdk8s-cli Sep 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working @component/cdk8s-cli Issue related to cdk8s-cli effort/small 1 day tops priority/p2 Dependent on community feedback. PR's are welcome :)
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Add YAML document separator to each manifest rassie/cdk8s-cli
3 participants
@rassie @iliapolo @sumupitchayan