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

fix(util): correct logging for synth #1634

Closed
wants to merge 9 commits into from
Closed

fix(util): correct logging for synth #1634

Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6188c90
correct logging for synth
iceglober Nov 10, 2023
881515d
typo
iceglober Nov 10, 2023
7f94c2f
Merge branch '2.x' into synth-dir-logging
iceglober Nov 10, 2023
530deab
Merge branch '2.x' into synth-dir-logging
iceglober Nov 11, 2023
c0f1fc4
Merge branch '2.x' into synth-dir-logging
iceglober Nov 11, 2023
c2fb72f
Merge branch '2.x' into synth-dir-logging
iceglober Nov 15, 2023
ec2cc28
Merge branch '2.x' into synth-dir-logging
iceglober Nov 16, 2023
b9fa399
Merge branch '2.x' into synth-dir-logging
iceglober Dec 6, 2023
9e3188c
Merge branch '2.x' into synth-dir-logging
iceglober Apr 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,9 +44,14 @@ export async function mkdtemp(closure: (dir: string) => Promise<void>) {
}

export async function synthApp(command: string, outdir: string, stdout: boolean, metadata: boolean): Promise<SynthesizedApp> {
if (!await fs.pathExists(outdir)) {
Copy link
Member

Choose a reason for hiding this comment

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

Putting this here means that this warning will always be printed because the outdir is expected to be created further down (in the await shell call).

console.log('WARNING: specified outdir does not exist, synth command could fail')
}

if (!stdout) {
console.log('Synthesizing application');
}

await shell(command, [], {
shell: true,
env: {
Expand All @@ -58,11 +63,6 @@ export async function synthApp(command: string, outdir: string, stdout: boolean,
},
});

if (!await fs.pathExists(outdir)) {
console.error(`ERROR: synthesis failed, app expected to create "${outdir}"`);
process.exit(1);
Copy link
Member

Choose a reason for hiding this comment

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

I understand where your coming from with having synth not fail in this case (because manifests were actually created). However, from the CLIs perspective, synthesis is not only creating manifests, there are additional CLI features (like validations) that rely on the fact this directory exists, and so from its POV, synthesis is not successful. This does unfortunately mean that, if you define outdir in your app, you must also change the output property in the cdk8s.yaml configuration file.

Note that if you do it the other way around, i.e set output: out in cdk8s.yaml, and leave the App instance with its defaults, everything will work as expected.

}

let found = false;
const yamlFiles = await findManifests(outdir);
if (yamlFiles?.length) {
Expand All @@ -76,6 +76,8 @@ export async function synthApp(command: string, outdir: string, stdout: boolean,

if (!found) {
console.error('No manifests synthesized');
} else {
console.log('Manifests synthesized!')
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this necessary though. If we found manifests, then we print their path already, indicating a successful synthesis.

}

const constructMetadata = findConstructMetadata(outdir);
Expand Down