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.

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

feat(cli): add cdktf watch command #817

Merged
merged 35 commits into from
Jul 29, 2021
Merged

Conversation

ansgarm
Copy link
Member

@ansgarm ansgarm commented Jul 1, 2021

Resolves #801

The initial implementation of the new cdktf watch command also contains an
experiment in regards to separating the underlying invocation of a synth and of
Terraform for init, plan and apply from the presentation layer (which is in our
case a CLI build with React components using ink). The other commands (e.g.
cdktf deploy) which we currently have, make use of React Hooks for
orchestrating the underlying steps that are required. Quite early it turned out
to be unreasonable to implement the dynamic nature of the watch command using
those same tools. Instead we opted for trying out a separate process and using a
GraphQL api to connect the two. The main reason for this (as opposed to e.g.
ProtoBuf or REST) was the superior tooling support that exists for this in
TypeScript and React. However, with this prototype now in the open, we will
carefully examine it and assess the pros and cons before jumping ship.

watch-lambda

@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch from 2021022 to 2cb0bdf Compare July 1, 2021 11:54
@ansgarm ansgarm changed the title Add cdktf watch command feat(cli): add cdktf watch command Jul 1, 2021
@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch from 2cb0bdf to 38b75eb Compare July 6, 2021 06:57
@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch 2 times, most recently from 41477ba to b38cac3 Compare July 13, 2021 19:22
@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch from 04e4933 to 4cb7006 Compare July 23, 2021 08:14
@ansgarm ansgarm requested a review from DanielMSchmidt July 23, 2021 08:50
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt left a comment

Choose a reason for hiding this comment

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

That's a really cool new feature 🙌

packages/cdktf-cli/bin/cmds/helper/synth-stack.ts Outdated Show resolved Hide resolved
packages/cdktf-cli/bin/cmds/ui/watch.tsx Show resolved Hide resolved
packages/cdktf-cli/bin/cmds/ui/watch.tsx Show resolved Hide resolved
packages/cdktf-cli/bin/cmds/watch.ts Outdated Show resolved Hide resolved
packages/cdktf-cli/bin/cmds/watch.ts Show resolved Hide resolved
packages/cdktf-cli/lib/client/react.tsx Show resolved Hide resolved
packages/cdktf-cli/lib/server/WatchClient.ts Outdated Show resolved Hide resolved
packages/cdktf-cli/lib/server/server.ts Outdated Show resolved Hide resolved
@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch 3 times, most recently from c917bd6 to d0ef883 Compare July 27, 2021 19:44
@ansgarm ansgarm marked this pull request as ready for review July 27, 2021 20:22
@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch 5 times, most recently from 5bc74d3 to 5a18a7c Compare July 28, 2021 17:48
@ansgarm ansgarm requested a review from DanielMSchmidt July 28, 2021 19:41
@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch from 5eeccb0 to b3c99bd Compare July 28, 2021 19:47
Copy link
Contributor

@DanielMSchmidt DanielMSchmidt left a comment

Choose a reason for hiding this comment

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

  • When I run this I see starting server and connecting messages. If I didn't read through the implementation I'd be confused to what server is starting and what it's connecting to.
  • I see errors twice currently

Screenshot 2021-07-29 at 13 57 33

- UI wise these should be in separate lines I beli

Screenshot 2021-07-29 at 13 59 04

eve - It does not show the last elements being deployed

Screenshot 2021-07-29 at 14 05 14

- Terraform assets seem to be not included in the watch, I changed sth in my application I deploy via NullResource and include via TerraformAsset but no update was triggered

docs/working-with-cdk-for-terraform/watch.md Show resolved Hide resolved
docs/working-with-cdk-for-terraform/watch.md Show resolved Hide resolved
packages/cdktf-cli/bin/cmds/ui/watch.tsx Outdated Show resolved Hide resolved
packages/cdktf-cli/bin/cmds/ui/watch.tsx Outdated Show resolved Hide resolved
packages/cdktf-cli/lib/server/WatchClient.ts Outdated Show resolved Hide resolved
packages/cdktf-cli/lib/server/WatchClient.ts Outdated Show resolved Hide resolved
packages/cdktf-cli/lib/server/WatchClient.ts Show resolved Hide resolved
packages/cdktf-cli/lib/server/server.ts Outdated Show resolved Hide resolved
packages/cdktf-cli/lib/server/server.ts Show resolved Hide resolved
@ansgarm
Copy link
Member Author

ansgarm commented Jul 29, 2021

I see errors twice currently

@DanielMSchmidt Have you set the CDKTF_LOG_LEVEL to something other than the default?

@DanielMSchmidt
Copy link
Contributor

@ansgarm I don't think so, I believe it was unset. But shouldn't the logs go only into the cdktf.log file anyways?

@ansgarm
Copy link
Member Author

ansgarm commented Jul 29, 2021

@ansgarm I don't think so, I believe it was unset. But shouldn't the logs go only into the cdktf.log file anyways?

@DanielMSchmidt If you set the log level but not the DISABLE_LOGGING flag, it will print the errors afaik

@DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt If you set the log level but not the DISABLE_LOGGING flag, it will print the errors afaik

Yeah that could have been the case :)

@ansgarm
Copy link
Member Author

ansgarm commented Jul 29, 2021

@DanielMSchmidt
Fixed multiple resources that were displayed side by side ✅
flex_direction

@ansgarm
Copy link
Member Author

ansgarm commented Jul 29, 2021

Also massive spoiler for the Office Hours later on 😉

@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch from 7916cd2 to 72663a3 Compare July 29, 2021 13:51
@ansgarm
Copy link
Member Author

ansgarm commented Jul 29, 2021

Terraform assets seem to be not included in the watch, I changed sth in my application I deploy via NullResource and include via TerraformAsset but no update was triggered

@DanielMSchmidt Is that file you changed by chance in your .gitignore?

Theoretically it should watch every file that is not in your .gitignore for synthesizing and it should watch all files in the working directory of your target stack for deploying.

@DanielMSchmidt
Copy link
Contributor

@DanielMSchmidt Is that file you changed by chance in your .gitignore?

No I referenced the docker backend with TerraformAsset that is in ../../application/backend and changed the index.js in there (in the docker e2e example)

ansgarm added 22 commits July 29, 2021 16:31
…equires node-pty which needs more advanced configuration to run on windows
…e can import it without changing our whole project
also fixes event emitter error when using the useStdoutDimensions hook too often – just use one and pass down the result
@ansgarm ansgarm force-pushed the 801-cdktf-watch-command branch from 92412b8 to fa23db5 Compare July 29, 2021 14:31
@DanielMSchmidt DanielMSchmidt merged commit 9f73f20 into main Jul 29, 2021
@DanielMSchmidt DanielMSchmidt deleted the 801-cdktf-watch-command branch July 29, 2021 15:15
@github-actions
Copy link
Contributor

github-actions bot commented Dec 9, 2022

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 9, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cdktf watch command
2 participants