-
Notifications
You must be signed in to change notification settings - Fork 40
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
Use connectionId in triggers for new gateway deployment #208
Conversation
parent: api, | ||
replaceOnChanges: ['*'], | ||
deleteBeforeReplace: true, |
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 will cause downtime, which could be ok if we are accepting that, but we will always have periods of BAD GATEWAY responses in this scenario
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.
We already do this for API Gateways here: https://github.com/klothoplatform/klotho/pull/208/files#diff-86a7f02a80cba7c191b1ed596a10dcceb360abb88a53be60c32bd9a195480d41R321-R344
We may want to fix it for this case too, but let's at least fix the tests then prioritize the downtime.
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.
that link takes me right to the addition of these lines so a little confused where its. supposed to take me to
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.
Guess it doesn't like linking to places outside of the diff context.
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.
ah i see, yeah i mean if this works then good enough for now, but we likely want to figure out a good way to solve this moving forward because it would be bad for a production env
@@ -372,6 +371,8 @@ export class ApiGateway { | |||
}, | |||
{ | |||
parent: method, | |||
replaceOnChanges: ['*'], | |||
deleteBeforeReplace: true, |
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.
same here about downtime.
can we run some of the upgrade tests to verify? The downtime may be acceptable because this should not be happening often but maybe a wider discussion |
// Vary the description to force recreation if the vpcLink changes | ||
// https://github.com/hashicorp/terraform/issues/6613#issuecomment-322264393 | ||
// https://github.com/hashicorp/terraform/issues/18392#issuecomment-402684841 | ||
description: pulumi.interpolate`Stage hash: ${sha256.sync(vpcLink.id)}`, |
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 is for v2API, the restAPI we use is v1, which is just in the create Docker Lambda function
da2e808
to
a9ba828
Compare
|
Should hopefully fix #202
Standard checks