-
Notifications
You must be signed in to change notification settings - Fork 4k
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
docs(apigateway): add warning about split stack technique #29691
docs(apigateway): add warning about split stack technique #29691
Conversation
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 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.
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.
Thanks for this PR! I made some comments.
@@ -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. |
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.
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.)
> **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. |
@@ -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. | |||
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). |
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.
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.
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). |
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.
This may not be necessary.
@@ -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. | |||
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). | |||
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). |
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 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.)
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.
(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...)
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.
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:
- (aws-apigateway): RestApi imported with method fromRestApiAttributes not updating current deployment in stage #13526 (comment)
- (apigateway): support deployment salting for imported RestApi #12417 (comment)
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?
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.
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.
Thank you for your review.
|
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.
Thanks for your changes. I have one last comment. I will approve this as soon as it is done.
> **Warning:** In the code above, an API Gateway deployment is created during the initial CDK deployment. | ||
However, if there are changes to the resources in subsequent CDK deployments, a new API Gateway deployment is not automatically created. As a result, the latest state of the resources is not reflected. | ||
To ensure the latest state of the resources is reflected, a manual deployment of the API Gateway is required after the CDK deployment. |
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.
It is good to insert line breaks at visually appropriate places in the document. (We can feel relief that these line breaks are not applied when they are actually displayed in the browser.)
> **Warning:** In the code above, an API Gateway deployment is created during the initial CDK deployment. | |
However, if there are changes to the resources in subsequent CDK deployments, a new API Gateway deployment is not automatically created. As a result, the latest state of the resources is not reflected. | |
To ensure the latest state of the resources is reflected, a manual deployment of the API Gateway is required after the CDK deployment. | |
> **Warning:** In the code above, an API Gateway deployment is created during the initial CDK deployment. | |
However, if there are changes to the resources in subsequent CDK deployments, a new API Gateway deployment is not | |
automatically created. As a result, the latest state of the resources is not reflected. To ensure the latest state | |
of the resources is reflected, a manual deployment of the API Gateway is required after the CDK deployment. |
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.
@go-to-k
Thank you! Made corrections!
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.
Thank you! Looks good to me.
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 update |
☑️ Nothing to do
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
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). |
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. |
Issue #29690
Closes #29690
Reason for this change
Regarding the stack separation of RestApi and Resource, there is no documentation about the fact that Deployment is not automatically created. When I actually add resources to the code documented and try cdk deploy for the second time and beyond, a new deployment is not created, and the latest resources are not reflected.
Description of changes
I added a note and related links to the documentation.
Description of how you validated changes
Nothing. It is just to change the description.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license