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

DescribeExecution as option #222

Merged
merged 5 commits into from
Jul 8, 2019
Merged

DescribeExecution as option #222

merged 5 commits into from
Jul 8, 2019

Conversation

shota
Copy link
Contributor

@shota shota commented Jul 4, 2019

As described in issue #72, DescribeExecution support is very important feature when implementing step functions with API Gateway. However, it seems like I can implement this with very short difference so I wrote this pull request.

Caution: This pull request makes "DescribeExecution" action allowed in the policy.

@shota
Copy link
Contributor Author

shota commented Jul 4, 2019

Example configuration as below

stepFunctions:
  stateMachines:
    command:
      events:
        - http:
            path: command/execute
            method: post
            cors: true
        - http:
            path: command/status
            method: post
            cors: true
            is_describe: true

"is_describe: true" makes us happy!

@theburningmonk
Copy link
Collaborator

@shota thanks for putting this together, I have a couple of feedback:

  1. I think the original config change proposed in Support DescribeExecution action in API Gateway #72 is more extensible, it'll be easier to support other actions like stopping an execution, etc. in the future
events:
   -
    http:
      path: status
      method: get
      cors: true
      action: DescribeExecution
  1. I notice you didn't have to update the integration request template, but StartExecution has different input argument to DescribeExecution, so how did you test this? e.g. what payload did you post to the describe endpoint to make it work?

  2. can you update the relevant section in the README as well to document the new feature?

  3. we use semantic-release to generate the new version number, so it relies on the commit messages to follow the conventional commit standard. Please update the commit messages so they follow the convention.

@shota
Copy link
Contributor Author

shota commented Jul 5, 2019

Sounds fantastic, I'll try again later.

@shota
Copy link
Contributor Author

shota commented Jul 5, 2019

@theburningmonk Thank you for your review, my pull request has some bugs and I fixed them and clean commit messages as you mentioned.

Document was fixed and code was tested with full coverage.


if (_.has(http, 'action')) {
// no template if some action was defined.
return {};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean any user-specified request template won't be used? e.g. if someone wants to use a GET to describe the execution and pass in the execution ARN as a query string param, would they be able to write a custom request template to do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, if user specify http.request.template option, it will be applied. These codes are just avobe return defaultTemplate. The default action for StartExecution should not be applied all other actions and should pass users action through.

Copy link
Collaborator

Choose a reason for hiding this comment

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

right, right, I see. Thanks, that looks good to me.

{
Effect: 'Allow',
Action: iamRoleApiGatewayToStepFunctionsAction,
Resource: '*',
Copy link
Collaborator

Choose a reason for hiding this comment

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

A minor thing, we can change this later in another PR, but we should be able to restrict the resource to just the state machine in question.

@theburningmonk theburningmonk merged commit 479d003 into serverless-operations:master Jul 8, 2019
@theburningmonk
Copy link
Collaborator

🎉 This PR is included in version 2.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@theburningmonk
Copy link
Collaborator

@shota I didn't realise you had a breaking change commit message in one of the commit messages before I merged. For future reference, it shouldn't count as a breaking change since it doesn't change any of the existing behaviours.

@horike37 so this PR has published a major version where it probably doesn't need to. I can delete the version on NPM and force semantic-release to publish it as 1.26.0 instead. What do you think?

@horike37
Copy link
Collaborator

horike37 commented Jul 8, 2019

@theburningmonk
Thank you for maintaining it as always 👍

I can delete the version on NPM and force semantic-release to publish it as 1.26.0 instead. What do you think?

It should cut a release as 1.26.0. Please force update 🙏

@shota
Great work 😄 This PR would make many users to be pleased.
ありがとうございましたー!!

@shota
Copy link
Contributor Author

shota commented Jul 8, 2019

@theburningmonk
Oh, I didn't recognize these message cause this, I understand now. Thank you for your support!

@horike37
🎉 Thank you! こちらこそ!

@theburningmonk
Copy link
Collaborator

@shota no worries, I should have paid more attention when reviewing :-P It's ok, it seems we can't unpublish v2.0.0 so we're just gonna leave it as it is. Thanks again for the contribution!

ss-betseqnzr pushed a commit to BetSEQNZR/serverless-step-functions that referenced this pull request Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants