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.

Sign up for GitHub

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

Jump to bottom

docs(apigateway): add warning about split stack technique #29691

Merged
merged 5 commits into from
Apr 18, 2024
Merged

docs(apigateway): add warning about split stack technique #29691

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a19219c
docs(aws-apigateway): add warning about split stack technique
engineer-taro Apr 2, 2024
83563fd
Merge branch 'main' into feat/add-warning-split-stack-api-gateway
engineer-taro Apr 2, 2024
d4dfaa8
docs(aws-apigateway): Warn about manual API Gateway deployment after …
engineer-taro Apr 14, 2024
874c6d6
docs(aws-apigateway): Add line breaks for better visual readability
engineer-taro Apr 15, 2024
a34c249
Merge branch 'main' into feat/add-warning-split-stack-api-gateway
aaythapa Apr 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions packages/aws-cdk-lib/aws-apigateway/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -267,6 +267,10 @@ The following example uses sets up two Resources '/pets' and '/books' in separat

[Resources grouped into nested stacks](test/integ.restapi-import.lit.ts)

> **Warning:** With the above code, no Deployment is created even if there are changes to the resources, and the latest resources are not reflected.
Copy link
Contributor

@go-to-k go-to-k Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

With this sentence, it may appear that a deployment will not be created even once... (But it will be created the first time.)

It would be also good to add a note that it needs to be deployed manually to reflect the latest resources.

And I think the Deployment might be good to be a lower case because it didn't seem to refer to the CDK resource (construct) name. (If you are referring to the CDK construct name Deployment, it would be better to enclose them in back quotation marks.)

Suggested change
> **Warning:** With the above code, no Deployment is created even if there are changes to the resources, and the latest resources are not reflected.
> **Warning:** With the above code, no deployment is created even if there are changes to the resources, and the latest resources are not reflected.

To create a Deployment, you need to use [`Deployment.addToLogicalId()`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_apigateway.Deployment.html#addwbrtowbrlogicalwbriddata).
Copy link
Contributor

Choose a reason for hiding this comment

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

If we provide a workaround of using Deployment.addToLogicalId(), it might be good to provide example code.

And added automatically here, because I wrote above that a deployment needs to be done manually.

Suggested change
To create a Deployment, you need to use [`Deployment.addToLogicalId()`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_apigateway.Deployment.html#addwbrtowbrlogicalwbriddata).
To create a deployment automatically, you need to use [`Deployment.addToLogicalId()`](https://docs.aws.amazon.com/cdk/api/v2/docs/aws-cdk-lib.aws_apigateway.Deployment.html#addwbrtowbrlogicalwbriddata).

Copy link
Contributor

Choose a reason for hiding this comment

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

This may not be necessary.

#29691 (comment)

Currently, it is possible to work around this issue with a workaround implementation. Please refer to the [related issue](https://github.com/aws/aws-cdk/issues/13526).
Copy link
Contributor

@go-to-k go-to-k Apr 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The reason I asked you to write specific workarounds in the above threads is because I thought that just posting the link in question would not be enough for users to know what to do.

If we write the workarounds, do we need to post the issue link here? Is there more information packed into the issue than that? (It's already closed, so just posting the link may cause users to close the page. If you want to include it, it might be a good idea to tell them what they should look for.)

Copy link
Contributor

Choose a reason for hiding this comment

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

(Is this a bug in the first place...? If it is a bug that needs to be resolved, perhaps the best course of action is to open a new issue and work on it. If it's not a bug, then I'm not sure, but maybe there's no need to post a link to the issue here...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@go-to-k

Thank you very much for the detailed review. It is challenging to determine whether this is a bug, as similar issues have been raised multiple times in the past. In those cases, workarounds have been shared, and the issues have either been closed or remain unresolved due to the time required to address them.

The reason why the deployment is not created is that the object obtained by RestApi.fromRestApiAttributes() does not have dynamic attributes such as latestDaployment.

Reference Issues:

The concern is that developers considering stack splitting may find it difficult to discover this problem.

The current issue with the documentation is that it shares the implementation of stack splitting where automatic deployment is not performed. Would it be appropriate to add the implementation of Deployment.addToLogicalId to the sample code?
However, even with this implementation, the issue of creating the deployment at the appropriate timing remains.

class DeployStack extends NestedStack {
  constructor(scope: Construct, props: DeployStackProps) {
    super(scope, 'integ-restapi-import-DeployStack', props);

    const deployment = new Deployment(this, 'Deployment', {
      api: RestApi.fromRestApiId(this, 'RestApi', props.restApiId),
    });
    if (props.methods) {
      for (const method of props.methods) {
        deployment.node.addDependency(method);
      }
    }
    const logicalId = // salted id
    deployment.addToLogicalId(logicalId)
    new Stage(this, 'Stage', { deployment });
  }
}

Alternatively, if this is a long-standing issue, should we wait for a resolution, even though the timeline is uncertain?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for clearing.

Would it be appropriate to add the implementation of Deployment.addToLogicalId to the sample code?
However, even with this implementation, the issue of creating the deployment at the appropriate timing remains.

I'm not sure, if the implementation is incomplete and remains the another issue, it may not be appropriate to recommend it... It would also be helpful to just state that deployment must be done manually when resources are changed. I think it's important to write only what we know completely.

Alternatively, if this is a long-standing issue, should we wait for a resolution, even though the timeline is uncertain?

I don't see the need to wait all, as there is no guarantee that the solution will be found, and in the meantime, there will be new users who will be in trouble.


## Integration Targets

Methods are associated with backend integrations, which are invoked when this
Expand Down