feat(stepfunctions): allow setting comment on state machine#25858

Merged

mergify[bot] merged 2 commits intoaws:mainfrom

mschelstastic:sfn_add_comment

Aug 2, 2023

Contributor

mschelstastic commented Jun 5, 2023

Just a small PR to allow setting top level comment on a state machine

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

gitpod-io bot commented Jun 5, 2023

github-actions bot added beginning-contributor p2 labels

aws-cdk-automation requested a review from a team

June 5, 2023 20:16

aws-cdk-automation previously requested changes

View reviewed changes

Collaborator

aws-cdk-automation left a comment

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.

mschelstastic force-pushed the sfn_add_comment branch from 962fcbc to d16caeb Compare

June 5, 2023 22:01

aws-cdk-automation dismissed their stale review

June 5, 2023 22:03

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

mschelstastic changed the title ~~feat(sfn): allow setting comment on state machine~~ feat(stepfunctions): allow setting comment on state machine

Contributor Author

mschelstastic commented Jun 6, 2023

this is "ready" but the build is failing for what appears to be an unrelated reason... going to mark as draft until i have the cycles to look into that more thoroughly (also going to update from main in case it was just a flake)

mschelstastic marked this pull request as draft

June 6, 2023 22:24

mschelstastic marked this pull request as ready for review

June 7, 2023 00:03

Contributor Author

mschelstastic commented Jun 7, 2023

build succeeded on re-run so good to go :)

Collaborator

aws-cdk-automation commented Jun 15, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

1 similar comment

Collaborator

aws-cdk-automation commented Jun 16, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Contributor Author

mschelstastic commented Jun 16, 2023

I probably won't have time to look into the merge conflict until next week. Let me know if I should just close this until then or not (donno if the new DefinitionBody.fromChainable stuff sets the sfn comment or not so need to test, but def don't have time this week.

Collaborator

aws-cdk-automation commented Jun 17, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

2 similar comments

Collaborator

aws-cdk-automation commented Jun 18, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

Collaborator

aws-cdk-automation commented Jun 19, 2023

This PR cannot be merged because it has conflicts. Please resolve them. The PR will be considered stale and closed if it remains in an unmergeable state.

mschelstastic force-pushed the sfn_add_comment branch from bf9cd08 to c3e8144 Compare

June 19, 2023 20:24

aws-cdk-automation added the pr/needs-community-review label

Contributor Author

mschelstastic commented Jun 22, 2023

Just wanted to follow up on this- would appreciate if someone has time to look at it thanks :)

vinayak-kukreja previously requested changes

View reviewed changes

Contributor

vinayak-kukreja left a comment

Hey, this looks great. Thank you for contributing.
I have just some NITs.

Tagging @iliapolo for a second opinion.

packages/aws-cdk-lib/aws-stepfunctions/lib/state-graph.ts Outdated

+                 * Read/write. Only makes sense on the top-level graph, subgraphs
+                 * do not support this feature.
+                 *
+                 * @default No comment

Contributor

vinayak-kukreja Jun 27, 2023

NIT: Could you change this to @default - No comment.

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

+                /**
+                 * Comment that describes this state machine
+                 *
+                 * @default No comment

Contributor

vinayak-kukreja Jun 27, 2023

Similar to earlier comment :)

Contributor Author

mschelstastic Jun 27, 2023

heh yup, np, updated :D

mergify bot dismissed vinayak-kukreja’s stale review

June 27, 2023 22:52

Pull request has been modified.

Contributor

vinayak-kukreja commented Jun 28, 2023

Thank you for addressing the feedback. :) LGTM, syncing with Eli to get a second opinion.

iliapolo reviewed

View reviewed changes

packages/aws-cdk-lib/aws-stepfunctions/lib/state-graph.ts Outdated

Comment on lines +41 to +42

		* Read/write. Only makes sense on the top-level graph, subgraphs
		* do not support this feature.

Contributor

iliapolo Jun 29, 2023 •

edited

Loading

I understand you followed the same pattern as the timeout property - but this behavior makes me a little uncomfortable and I think that timeout also wasn't implemented in an ideal way. Lets see if we can make it better?

See other comments.

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

                 public bind(scope: Construct, sfnPrincipal: iam.IPrincipal, sfnProps: StateMachineProps): DefinitionConfig {
                   const graph = new StateGraph(this.chainable.startState, 'State Machine definition');
                   graph.timeout = sfnProps.timeout;
+                  graph.comment = sfnProps.comment;

Contributor

iliapolo Jun 29, 2023

What if instead we do:

const graphJson = Stack.of(scope).toJsonString(graph.toGraphJson());
return {
  definitionString: { Comment: sfnProps.comment, ...graphJson },
};

Basically having the state machine augment the graph with properties that are only relevant to the state machine as a whole - and not to sub-graphs.

Contributor Author

mschelstastic Jul 31, 2023

finally updated,

    const graphJson = graph.toGraphJson();
    return {
      definitionString: Stack.of(scope).toJsonString({ ...graphJson, Comment: sfnProps.comment }),
    };

how's that? I included comment after since the graph always sets it (even if undefined) and {something: 'foo', ... {something:undefined}} will override with undefined

vinayak-kukreja removed the pr/needs-community-review label

aws-cdk-automation added the pr/needs-community-review label

kaizencc previously requested changes

View reviewed changes

Contributor

kaizencc left a comment

@curquhart leaving a request changes review to reflect the current state of this PR and remove the needs-review label. We'll take another look when outstanding comments are addressed!

kaizencc removed the pr/needs-community-review label

Contributor Author

mschelstastic commented Jul 13, 2023

@curquhart leaving a request changes review to reflect the current state of this PR and remove the needs-review label. We'll take another look when outstanding comments are addressed!

sure thing. Sorry haven't addressed yet, I'm on vaca at the moment, should address it next week.

github-actions bot mentioned this pull request

Monthly PRs metrics report - June 2023 #26516

Closed

mschelstastic force-pushed the sfn_add_comment branch from e699785 to ab90a05 Compare

July 31, 2023 19:01

mergify bot dismissed kaizencc’s stale review

July 31, 2023 19:02

Pull request has been modified.

aws-cdk-automation added the pr/needs-community-review label

kaizencc previously requested changes

View reviewed changes

Contributor

kaizencc left a comment

Hi @curquhart! Thanks for sticking with this PR. One last comment about the changes to StateGraph.

packages/aws-cdk-lib/aws-stepfunctions/lib/state-graph.ts Outdated

                     StartAt: this.startState.stateId,
                     States: states,
                     TimeoutSeconds: this.timeout && this.timeout.toSeconds(),
+                    Comment: this.comment,

Contributor

kaizencc Aug 1, 2023

I don't think changes to StateGraph are necessary anymore. StateGraph is exposed but not meant to be used externally. No one should be setting comment on StateGraph and internally we are not doing that anymore either. So there's no need to have this prop.

(This means that the ordering between ...graphJson and Comment: sfnProps.comment won't matter anymore, but its fine to have graphJson in front).

Contributor Author

mschelstastic Aug 1, 2023

haha... I forgot i added it to state-graph, totally makes sense to take it out - updated

aws-cdk-automation removed the pr/needs-community-review label

mschelstastic force-pushed the sfn_add_comment branch from ab90a05 to 94a2dea Compare

August 1, 2023 15:32

mergify bot dismissed kaizencc’s stale review

August 1, 2023 15:32

Pull request has been modified.


          feat(stepfunctions): allow setting comment on state machine

42495e9

mschelstastic force-pushed the sfn_add_comment branch from 94a2dea to 42495e9 Compare

August 1, 2023 15:33

aws-cdk-automation added the pr/needs-community-review label

kaizencc approved these changes

View reviewed changes

Contributor

kaizencc left a comment

Thanks @curquhart! Looks good

Contributor

mergify bot commented Aug 2, 2023

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).


          Merge branch 'main' into sfn_add_comment

dc348a3

aws-cdk-automation removed the pr/needs-community-review label

Collaborator

aws-cdk-automation commented Aug 2, 2023

AWS CodeBuild CI Report

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

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

mergify bot merged commit 6a70b4f into aws:main

Contributor

mergify bot commented Aug 2, 2023

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).

mschelstastic deleted the sfn_add_comment branch

April 17, 2024 03:48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

beginning-contributor p2