-
Notifications
You must be signed in to change notification settings - Fork 777
feat(stepfunctions): Show state machine executions in the Explorer view. #7847
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: master
Are you sure you want to change the base?
Conversation
|
⏳ I'm reviewing this pull request for security vulnerabilities and code quality issues. I'll provide an update when I'm done |
|
|
✅ I finished the code review, and didn't find any security or code quality issues. |
ac3578c to
cb63aa1
Compare
|
Hi @zelzhou and @anthonyting, I recently submitted this PR, but then I saw you're also doing some active work on displaying Step Functions executions. I don't think my change overlaps with your change, but I'd love your feedback on this PR. Hopefully you're willing to accept community contributions, especially from a former colleague :-) |
| getNoChildrenPlaceholderNode: async () => | ||
| new PlaceholderNode( | ||
| this, | ||
| localize('AWS.explorerNode.stepfunctions.noStateMachineExecution', '[No Executions found]') |
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.
The new translation keys that are introduced (including the ones below) need to be added to the localisation file - core/package.nls.json
Also, for the translation key construction logic, I belive the toolkit team prefers to have a service name as a higher-order node going after AWS, like our existing keys: AWS.stepFunctions....
Though I don't have a strong opinion on that, I am good to keep these as is if the toolkit team is fine with it
|
Hi @peter-smith-phd, I apologize on the team's behalf for taking so long to review your PR - we'll be on top of your future contributions if you are still up to extending the functionality as stated in the description. To provide some context, we are currently working on introducing execution details page on the toolkit, but we will be happy to accept contributions that improve overall experience of working with SFN via toolkit. The current PR does not overlap with our change and I really love the feature you are adding here! Let me comment on your future ideas to rule out potential conflicts:
A couple of additional comments about current CR:
I will also ask someone else for SFN to review this PR |
|
Hi @witness-me, thanks for the feedback. It's been a while since I looked at this PR, but I'll review it again next week and make your recommended updates. Thanks! |
| const dateA = nodeA.details.startDate as Date // startDate will never be undefined. | ||
| const dateB = nodeB.details.startDate as Date |
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.
wondering if we should use satisfies keyword here?
| */ | ||
| export class StateMachineNode extends AWSTreeNodeBase implements AWSResourceNode { | ||
| public static readonly contextValue = 'awsStateMachineNode' | ||
| public static readonly maxExecutionsToShow = 10 |
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.
thinking about the UX here - I think 10 is a good default but should we offer any ways to filter this to specific execution statuses? wondering if beside the state machine name we could show a filter and/or more options to customize what executions show up.
| * Assuming the state machine execution was started via a StateMachineNode in the | ||
| * AWS Explorer, refresh the list of executions, so this new execution will appear. | ||
| */ | ||
| private async refreshExplorerNode(node?: StateMachineNode): Promise<void> { |
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.
nice - I don't see anything that periodically polls the running executions, I think that could be a useful addition too.
cb63aa1 to
53f02cf
Compare
|
Sorry, accidentally closed this PR when I was rebasing (with 6 months of changes). I'll shortly re-test and make changes based on feedback. |
Problem
When developing with AWS Step Functions, the current UI allows the user to start a state machine execution via the
context menu in the AWS Explorer view. However, once the execution has started, it's not possible to view the execution
details, or to learn whether the execution succeed, failed, or is still running. Additionally, developers will want to view the final output of the state machine, as well as the execution history steps. These steps are all necessary for them to proactively debug their state machine code.
Solution
This PR adds state machine executions as child nodes underneath the state machine name in the AWS Explorer. When a state machine execution is started, the new execution is shown (in running state). Up to 10 of the most recent executions are shown in the explorer, which should be enough given that developers will be manually running and validating their state machine, rather than running them at scale.
In future PRs, I'd like to add:
This current PR is the basis for this future work.
feature/xbranches will not be squash-merged at release time.