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

[StaticSite]: Please make atomic deploys 'optional' #1100

Open
iDVB opened this issue Dec 1, 2021 · 7 comments
Open

[StaticSite]: Please make atomic deploys 'optional' #1100

iDVB opened this issue Dec 1, 2021 · 7 comments

Comments

@iDVB
Copy link
Contributor

iDVB commented Dec 1, 2021

Currently, each time even when the smallest copy change is made to our frontend code, SST needs to update the deployId in the CF Distro which costs a minimum of +6mins of replication time.

This is fine for a production deploy, but for a dev/feat deploy it would be ideal to choose to disable it and instead just do a simple aws s3 sync --delete to push all files into the root of the bucket and delete any orphans. This would result is much faster non-prod deploys.

Somewhat related to #1099

@dkershner6
Copy link
Contributor

I've debated on how best to handle this myself (Dev/Preview environments, mostly around the Next construct, but it's a similar concept). I am not intimately familiar with the particulars of implementation, so I could be wrong below.

I think I have mostly settled my own opinion on that this should likely be another construct that shares a bit of functionality with the main construct. It changes the deployment pretty fundamentally, and if you tried to change the setting from one to another, it could rapidly become VERY complex, especially in the more complex constructs (like Next).

I do, however, like the idea of a "throwaway" deploy using CFn. I think using SDK only could get tedious.

@iDVB
Copy link
Contributor Author

iDVB commented Dec 1, 2021

For what its worth.. we've been doing our frontend code deploy non-atomically for years for dev/feat without issue.
I don't think it needs to be a complex change.

Just have a boolean on StaticSite props that defaults to atomicDeploys=true but allows a developer to set it to false.
Then, pushing a code change would only do two things differently is NOT atomic:

  1. Make the deployId = / instead of the dynamic key
  2. Make the S3Uploader code delete orphan files OR empty the bucket first, maybe by passing atomicDeploys boolean in as envVar.

@dkershner6
Copy link
Contributor

dkershner6 commented Dec 1, 2021

I worry about how the atomic deploy would check if there were any non-atomic deploys...but it's possible that doesnt matter and there would just be random files in the root of the bucket. The other direction does indeed seem simpler, as you can just delete it all.

A new construct would just avoid any of these complications and doesnt really have a downside (at least that I can see).

@iDVB
Copy link
Contributor Author

iDVB commented Dec 1, 2021

@dkershner6 Not sure what the scenario is that "atomic deploy would check if there were any non-atomic deploys"?
The entire stack including the uploader (that does the deploys), the bucket itself and the contents of the bucket are all ephemeral. Unless you are changing this value after the stack has been deployed, which is not what I'm suggesting.

There would only ever be two kinds of stacks:

  • ones with a StaticSite bucket that is deployed to atomically
  • ones with a StaticSite bucket that is deployed to at the root

Never would there be a stack/bucket that has both.

@dkershner6
Copy link
Contributor

dkershner6 commented Dec 1, 2021

There would only ever be two kinds of stacks:
ones with a StaticSite bucket that is deployed to atomically
ones with a StaticSite bucket that is deployed to at the root
Never would there be a stack/bucket that has both.

I understood your use case, but to suggest these things are true you would either need:

  • Two separate constructs (so Cfn would just recognize them as totally different things and would throw id conflict errors if you tried to switch)
  • Some kind of mechanism to check for changes and throw errors if someone did change this setting.

Cfn is not a fan of the honor system. ;)

I agree with you on what, why, just see a potential hole in how, @iDVB .

@iDVB
Copy link
Contributor Author

iDVB commented Dec 1, 2021

Ok, I see the scenario now. Just a way to ensure that IF someone changed the setting after deploy.... what would happen.
Is there not a way with CDK to force a s3 bucket recreate if that value changes? Similar to changing a property of CDN causing the CDN to come down and redeploy.

@dkershner6
Copy link
Contributor

Is there not a way with CDK to force a s3 bucket recreate if that value changes? Similar to changing a property of CDN causing the CDN to come down and redeploy.

I believe redeploy !== recreate (redeploy maintains distributionId and such), but maybe there is a way to empty the bucket?

Detecting the change is another issue as well though, as you don't really have access to "previous" that I know of without building something custom.

Regardless, I'm now out of my depth and I'll let the pros answer those ones.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants