-
Notifications
You must be signed in to change notification settings - Fork 23
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
fix(util): correct logging for synth #1634
base: 2.x
Are you sure you want to change the base?
Conversation
Signed-off-by: Austin Hess <austin@kayn.ai>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@iceglober Appreciate the work here, but we can't accept this change because it inherently changes the behavior of the CLI. Also as I explain in the comments, the current behavior is the desired one, albeit somewhat confusing.
Thanks!
@@ -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)) { |
There was a problem hiding this comment.
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).
@@ -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!') |
There was a problem hiding this comment.
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.
@@ -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); |
There was a problem hiding this comment.
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.
Apologies for the long delay in reviewing this. |
FYI - This is my first OSS contribution. I'm open to any feedback or input!
Summary of Changes
The console logs generated during
cdk8s synth
gave the impression that synthesis had failed when it actually succeeded, leading to unnecessary confusion. This change updates that log to a warning if theoutdir
doesn't exist, and also adds a "success" log if generated manifests are found. This isn't actually a problem becausesynth
in cdk8s-core always attempts to create the necessary directory.Current State
Without providing a CLI option for
outdir
and without specifying anoutdir
property in my config file, I defined my App like so:where
outdir
is defined in my IaC directly. I wanted to make a clear differentiation between TS output ("dist") andcdk8s
output (I chose "out").With this current state, here's what the console output looks like:
This "ERROR" message gives the false impression that manifest synthesis failed, when in reality, because I had specified a different
outdir
in my TS file, everything worked just fine.Proposed Change
My changes make it so that the console output reflects the following:
This accomplishes the following:
pathExists
check resulted in an exit)