-
Notifications
You must be signed in to change notification settings - Fork 471
fix(cli): improve no stacks found message in synth output #3864
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
base: main
Are you sure you want to change the base?
fix(cli): improve no stacks found message in synth output #3864
Conversation
39b12d9
to
61671c1
Compare
c5e46ac
to
1d6779f
Compare
8ce552f
to
139b2c2
Compare
fb853c7
to
f1f73d0
Compare
f1f73d0
to
dafac64
Compare
fix tests
6e0a4a2
to
9563025
Compare
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.
Looks good to me, @nithishravindra 👍
? `Generated Terraform code for the stacks: ${stacks | ||
.map((s) => s.name) | ||
.join(", ")}` | ||
: "No stacks found in configuration."} |
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 think I'd refer to a specific configuration (i.e. the one at hand) and therefore add a "the" to make that more specific. But that's mostly my personal choice, and I'd be open to skip this or get some feedback from a native speaker :)
: "No stacks found in configuration."} | |
: "No stacks found in the configuration."} |
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 think the existing one is slightly better, but also just a personal choice. Non-native speaker
|
||
test("SynthOutput", () => { | ||
const { lastFrame } = render(<SynthOutput stacks={[]} />); | ||
expect(stripAnsi(lastFrame())).toBe("No stacks found in configuration."); |
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.
(if we change the wording, this needs an update as well)
expect(stripAnsi(lastFrame())).toBe("No stacks found in configuration."); | |
expect(stripAnsi(lastFrame())).toBe("No stacks found in the configuration."); |
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.
Minor nits, but overall good to go! Thank you for adding tests for this too!
? `Generated Terraform code for the stacks: ${stacks | ||
.map((s) => s.name) | ||
.join(", ")}` | ||
: "No stacks found in configuration."} |
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 think the existing one is slightly better, but also just a personal choice. Non-native speaker
test("SynthOutput", () => { | ||
const { lastFrame } = render(<SynthOutput stacks={[]} />); | ||
expect(stripAnsi(lastFrame())).toBe("No stacks found in configuration."); | ||
{ |
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'd rather this be split into two tests. While it is completely fine, I'm personally not a big fan of restricting scope like this. That being said, if you want there to be just one test, you can use let
instead of const
. We do that a lot where we want to test a variety of small changes within the same test.
Related issue
Fixes #2793
Description
Updates the error message shown when no stacks are found during
cdktf synth
to be more descriptive. This helps users understand they need to define at least one TerraformStack in their CDKTF applicationChecklist