fix(stepfunctions): disabling logging still requires LogGroup by Tietew · Pull Request #30816 · aws/aws-cdk

Tietew · 2024-07-10T10:20:43Z

Issue # (if applicable)

Reason for this change

To disable logging on a StateMachine (with logging enabled), we should specify LogLevel.OFF to LogOptions.level. But cannot remove the LogGroup because LogOptions.destination is required.

new sfn.StateMachine(this, 'StateMachine', {
  definitionBody: ...,
  logs: { level: sfn.LogLevel.OFF } // allow to disable logging
});

Description of changes

Make LogOptions.destination optional.
Validate LogOptions.destination is present when LogOptions.level is not OFF.

Description of how you validated changes

Unit and integ tests that verify LogOptions.destination is opitional when LogOptions.level is OFF and throw an exception otherwise.

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

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

packages/aws-cdk-lib/aws-stepfunctions/lib/state-machine.ts

ChakshuGupta13 · 2024-07-15T16:42:01Z

packages/aws-cdk-lib/aws-stepfunctions/lib/state-machine.ts

+   * @default No log group. Required if your log level is not set to OFF.
   */
-  readonly destination: logs.ILogGroup;
+  readonly destination?: logs.ILogGroup;


will this not be a breaking change for existing usage? example:

const logOptions: LogOptions = { destination: <LogGroup> } const stateMachine: StateMachine = new StateMachine(parent, 'ID', { logs: logOptions, ... } ... stateMachine.logs.destination.logGroupName // Error stateMachine.logs.destination!.logGroupName // Change needed

Some user(s) might be utilising ILogGroup's public properties from logOptions which will now cause issues because now such users will need to make non-null assertions before using such properties (e.g. with !)

According to the jsii-diff documentation:

You are allowed to make inputs optional

So i think this change is not a breaking change.

I think @Tietew is right here. The example Chakshu mentioned is output property while the changed code is for input property.

ChakshuGupta13 · 2024-07-15T16:44:22Z

packages/aws-cdk-lib/aws-stepfunctions/test/state-machine.test.ts

+
+    expect(() => {
+      new sfn.StateMachine(stack, 'MyStateMachine', {
+        definitionBody: sfn.DefinitionBody.fromChainable(sfn.Chain.start(new sfn.Pass(stack, 'Pass'))),


can we not directly pass new sfn.Pass(stack, 'Pass') to DefintionBody.fromChainable as Pass implements IChainable?

it's just a copy & paste.
README also uses sfn.Chain.start() instead of bare sfn.Pass.

…-logging

codecov · 2024-11-20T04:18:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.16%. Comparing base (7be4456) to head (e290d40).

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #30816   +/-   ##
=======================================
  Coverage   82.16%   82.16%           
=======================================
  Files         119      119           
  Lines        6857     6857           
  Branches     1157     1157           
=======================================
  Hits         5634     5634           
  Misses       1120     1120           
  Partials      103      103

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

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

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

GavinZZ

LGTM, thank you and apologize for the wait on the review!

GavinZZ · 2025-02-14T23:26:23Z

packages/aws-cdk-lib/aws-stepfunctions/lib/state-machine.ts

+   * @default No log group. Required if your log level is not set to OFF.
   */
-  readonly destination: logs.ILogGroup;
+  readonly destination?: logs.ILogGroup;


I think @Tietew is right here. The example Chakshu mentioned is output property while the changed code is for input property.

mergify · 2025-02-14T23:30:54Z

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-02-14T23:35:48Z

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-02-15T01:47:03Z

This pull request has been removed from the queue for the following reason: pull request branch update failed.

The pull request can't be updated

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

Tietew · 2025-02-15T03:32:43Z

@Mergifyio update

mergify · 2025-02-15T03:32:51Z

update

❌ Mergify doesn't have permission to update

Details

For security reasons, Mergify can't update this pull request. Try updating locally.
GitHub response: refusing to allow a GitHub App to create or update workflow .github/workflows/analytics-metadata-updater.yml without workflows permission

aaythapa · 2025-02-18T17:21:06Z

@mergify update

mergify · 2025-02-18T17:21:32Z

update

☑️ Nothing to do

Details

#commits-behind > 0 [📌 update requirement]
-closed [📌 update requirement]
-conflict [📌 update requirement]
queue-position = -1 [📌 update requirement]

Pull request has been modified.

aws-cdk-automation · 2025-02-18T17:50:56Z

AWS CodeBuild CI Report

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

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

mergify · 2025-02-18T17:51:17Z

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-02-18T17:51:40Z

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.

make LogOptions.destination optional

b62fe80

github-actions bot added bug This issue is a bug. p2 labels Jul 10, 2024

aws-cdk-automation requested a review from a team July 10, 2024 10:20

github-actions bot added the admired-contributor [Pilot] contributed between 13-24 PRs to the CDK label Jul 10, 2024

aws-cdk-automation previously requested changes Jul 10, 2024

View reviewed changes

This comment was marked as resolved.

Sign in to view

aws-cdk-automation added the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Jul 10, 2024

add @default

3efbfba

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 11, 2024

add an intg test

3ca2073

github-actions bot mentioned this pull request Jul 15, 2024

Weekly PR metrics report #30858

Closed

ChakshuGupta13 suggested changes Jul 15, 2024

View reviewed changes

Tietew added 2 commits July 18, 2024 11:43

review

f5b6d2c

Merge remote-tracking branch 'origin/main' into stepfunctions-disable…

6a0fb67

…-logging

github-actions bot mentioned this pull request Aug 1, 2024

Monthly PR metrics report #30999

Closed

Tietew requested a review from ChakshuGupta13 August 14, 2024 02:54

Merge branch 'main' into stepfunctions-disable-logging

6dc606e

Tietew changed the title ~~fix(stepfunctions): make LogOptions.destination optional~~ fix(stepfunctions): disabling logging still requires LogGroup Aug 22, 2024

github-actions bot added the effort/small Small work item – less than a day of effort label Aug 22, 2024

Tietew added 2 commits August 27, 2024 14:11

Merge branch 'main' into stepfunctions-disable-logging

ef79c2c

Merge branch 'main' into stepfunctions-disable-logging

e0aacb9

gracelu0 removed the pr/reviewer-clarification-requested The contributor has requested clarification on feedback, a failing build, or a failing PR Linter run label Dec 24, 2024

GavinZZ self-assigned this Feb 14, 2025

GavinZZ previously approved these changes Feb 14, 2025

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Feb 14, 2025

Merge branch 'main' into stepfunctions-disable-logging

e290d40

aaythapa approved these changes Feb 18, 2025

View reviewed changes

mergify bot merged commit a2cd7ae into aws:main Feb 18, 2025
22 checks passed

github-actions bot locked as resolved and limited conversation to collaborators Feb 18, 2025

Tietew deleted the stepfunctions-disable-logging branch February 18, 2025 22:29

Conversation

Tietew commented Jul 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

This comment was marked as resolved.

Uh oh!

Uh oh!

ChakshuGupta13 Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

Tietew Jul 16, 2024

Choose a reason for hiding this comment

Uh oh!

GavinZZ Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

ChakshuGupta13 Jul 15, 2024

Choose a reason for hiding this comment

Uh oh!

Tietew Jul 18, 2024

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Nov 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

GavinZZ left a comment

Choose a reason for hiding this comment

Uh oh!

GavinZZ Feb 14, 2025

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Feb 14, 2025

Uh oh!

mergify bot commented Feb 14, 2025

Uh oh!

mergify bot commented Feb 15, 2025

Uh oh!

Tietew commented Feb 15, 2025

Uh oh!

mergify bot commented Feb 15, 2025

❌ Mergify doesn't have permission to update

Uh oh!

aaythapa commented Feb 18, 2025

Uh oh!

mergify bot commented Feb 18, 2025

☑️ Nothing to do

Uh oh!

aws-cdk-automation commented Feb 18, 2025

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Feb 18, 2025

Uh oh!

Uh oh!

github-actions bot commented Feb 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Tietew commented Jul 10, 2024 •

edited

Loading

codecov bot commented Nov 20, 2024 •

edited

Loading