chore(toolkit): introduce cloudwatchlogsmonitor to deploy and watch #33219

kaizencc · 2025-01-28T22:20:58Z

Issue # (if applicable)

Closes #.

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

packages/@aws-cdk/toolkit/test/actions/deploy.test.ts

codecov · 2025-01-28T22:32:48Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 80.84%. Comparing base (a486bbc) to head (2d70332).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #33219   +/-   ##
=======================================
  Coverage   80.84%   80.84%           
=======================================
  Files         232      232           
  Lines       14135    14135           
  Branches     2460     2460           
=======================================
  Hits        11428    11428           
  Misses       2427     2427           
  Partials      280      280

Flag	Coverage Δ
suite.unit	`80.84% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
packages/aws-cdk	`79.64% <ø> (ø)`
packages/aws-cdk-lib/core	`82.17% <ø> (ø)`

mrgrain · 2025-01-30T13:07:48Z

packages/@aws-cdk/toolkit/lib/actions/deploy/index.ts

+   *
+   * @default - not monitoring CloudWatch logs
+   */
+  readonly cloudWatchLogMonitor?: CloudWatchLogEventMonitor;


Do we really want to make this a public interface? What is the benefit of providing this as a customizable class?
Or would be better off turning this into a boolean option streamLogs?: boolean?

If we do want this as a public interface, it will have to be an ICloudWatchLogEventMonitor etc.

mrgrain · 2025-01-30T13:08:18Z

packages/@aws-cdk/toolkit/lib/actions/deploy/index.ts

+   *
+   * @default - not monitoring CloudWatch logs
+   */
+  readonly cloudWatchLogMonitor?: CloudWatchLogEventMonitor;


If we do want this as a public interface, it will have to be an ICloudWatchLogEventMonitor etc.

kaizencc · 2025-01-30T17:48:06Z

packages/@aws-cdk/toolkit/lib/actions/deploy/index.ts

+   *
+   * @default - not monitoring CloudWatch logs
+   */
+  readonly cloudWatchLogMonitor?: CloudWatchLogEventMonitor;


this is under ExtendedDeployOptions which is only used by watch and not used by customers. therefore i think it's fine/appropriate for it to be typed CloudWatchLogEventMonitor without the I because it is meant to always be the CloudWatchLogEventMonitor class.

mrgrain · 2025-01-30T18:22:26Z

packages/@aws-cdk/toolkit/lib/actions/deploy/index.ts

+   * Whether to show logs from all CloudWatch log groups in the template
+   * locally in the users terminal


Not sure what, but the Toolkit doesn't really have a concept of a user terminal. It works with the IoHost instead (which or might not print).

I realize we copied this description from elsewhere. But hey, let's make it better now!

good call out

mrgrain · 2025-01-30T18:25:32Z

packages/@aws-cdk/toolkit/lib/api/aws-cdk.ts

+export { findCloudWatchLogGroups } from '../../../../aws-cdk/lib/api/logs/find-cloudwatch-logs';
+
+// Test APIs
+export { MockSdk } from '../../../../aws-cdk/test/util/mock-sdk';


Okay that import needs to go elsewhere. Apologies, I didn't explain the purpose of this file well enough.

Basically we are using it as bundling entrypoint. Everything in here will be bundled for distribution. We don't need to MockSdk bundled.

your linter rule pointed me here :). but i'll make an exception for testing imports in that rule

packages/@aws-cdk/toolkit/lib/api/io/private/codes.ts

mrgrain

Approved in principle. We mainly need to fix the MockSdk import.

mrgrain

do-not-merge so you can do the fixups

Co-authored-by: Momo Kornher <[email protected]>

mergify · 2025-01-30T20:17:49Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2025-01-30T20:22:19Z

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #33219 has been dequeued. The pull request rule doesn't match anymore

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

kaizencc · 2025-01-30T21:20:52Z

@Mergifyio requeue

mergify · 2025-01-30T21:21:19Z

requeue

❌ This pull request head commit has not been previously disembarked from queue.

aws-cdk-automation · 2025-01-30T21:50:19Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: 2d70332
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2025-01-30T21:50:42Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions · 2025-01-30T21:51:05Z

Comments on closed issues and PRs are hard for our team to see.
If you need help, please open a new issue that references this one.

chore(toolkit): introduce cloudwatchlogsmonitor to deploy and watch

5eca5fe

github-actions bot added the p2 label Jan 28, 2025

aws-cdk-automation requested a review from a team January 28, 2025 22:21

kaizencc commented Jan 28, 2025

View reviewed changes

packages/@aws-cdk/toolkit/test/actions/deploy.test.ts Outdated Show resolved Hide resolved

mergify bot added the contribution/core This is a PR that came from AWS. label Jan 28, 2025

kaizencc and others added 2 commits January 29, 2025 14:40

Merge branch 'main' into conroy/cwlogs

fbc2a7a

tests

dd914b0

kaizencc marked this pull request as ready for review January 29, 2025 22:55

Merge branch 'main' into conroy/cwlogs

a7ae168

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 29, 2025

mrgrain reviewed Jan 30, 2025

View reviewed changes

mrgrain requested changes Jan 30, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jan 30, 2025

pr feedback

96034fb

kaizencc commented Jan 30, 2025

View reviewed changes

kaizencc and others added 3 commits January 30, 2025 12:48

name

3614245

Merge branch 'main' into conroy/cwlogs

fdd0c88

fix merge

1fa22e6

mrgrain reviewed Jan 30, 2025

View reviewed changes

packages/@aws-cdk/toolkit/lib/api/io/private/codes.ts Outdated Show resolved Hide resolved

mrgrain added the pr/do-not-merge This PR should not be merged at this time. label Jan 30, 2025

mrgrain requested changes Jan 30, 2025

View reviewed changes

mrgrain approved these changes Jan 30, 2025

View reviewed changes

kaizencc and others added 3 commits January 30, 2025 14:27

Update packages/@aws-cdk/toolkit/lib/api/io/private/codes.ts

a024fd3

Co-authored-by: Momo Kornher <[email protected]>

Merge branch 'main' into conroy/cwlogs

8c5bebb

move mocksdk to test side

4941dc6

kaizencc removed the pr/do-not-merge This PR should not be merged at this time. label Jan 30, 2025

Merge branch 'main' into conroy/cwlogs

2d70332

mergify bot merged commit 71c492a into main Jan 30, 2025
20 checks passed

mergify bot deleted the conroy/cwlogs branch January 30, 2025 21:50

github-actions bot locked as resolved and limited conversation to collaborators Jan 30, 2025

		* Whether to show logs from all CloudWatch log groups in the template
		* locally in the users terminal

chore(toolkit): introduce cloudwatchlogsmonitor to deploy and watch #33219

chore(toolkit): introduce cloudwatchlogsmonitor to deploy and watch #33219

Uh oh!

Conversation

kaizencc commented Jan 28, 2025

Issue # (if applicable)

Reason for this change

Description of changes

Describe any new or updated permissions being added

Description of how you validated changes

Checklist

Uh oh!

Uh oh!

codecov bot commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mrgrain Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

mrgrain Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

mrgrain Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

kaizencc Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

mrgrain Jan 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kaizencc Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

mrgrain Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

kaizencc Jan 30, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

mrgrain left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Jan 30, 2025

Uh oh!

mergify bot commented Jan 30, 2025

Uh oh!

kaizencc commented Jan 30, 2025

Uh oh!

mergify bot commented Jan 30, 2025

❌ This pull request head commit has not been previously disembarked from queue.

Uh oh!

aws-cdk-automation commented Jan 30, 2025

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Jan 30, 2025

Uh oh!

Uh oh!

github-actions bot commented Jan 30, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 28, 2025 •

edited

Loading

mrgrain Jan 30, 2025 •

edited

Loading