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

Add a bucket to the provider for deployments. #2189

Merged
merged 12 commits into from Oct 3, 2016
Merged

Add a bucket to the provider for deployments. #2189

merged 12 commits into from Oct 3, 2016

Conversation

DavidTanner
Copy link

@DavidTanner DavidTanner commented Sep 22, 2016

What did you implement:

_Implementing Issue:_ #1513

How did you implement it:

Added a check for deploymentBucket on the provider object. If it is there, then remove the S3 resource from the CloudFormation setup. Also all deploys are now prefixed with serverless/${serviceName}/ so multiple deploys can happen without deleting other services.

How can we verify it:

Use a config similar to this:

service: pr1513
provider:
  name: aws
  runtime: nodejs4.3
  stage: test
  region: us-west-2
  deploymentBucket: com.serverless.deploys

After the deploy you will see the S3 resource created initially, then removed from the CF file and the bucket created and deleted, finally in com.serverless.deploys there will be a new folder serverless/pr1513/${dateString}/ that contains the deployment files.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config/commands/resources
  • Leave a comment that this is ready for review once you've finished the implementation

Also prefix deployments so they can co-exist with other files

Also prefix deployments so they can co-exist with other files
Check that the bucket exists before, and that it is in the correct stage
Found some unused code when looking for throttling errors, added functionality to get code from SDK errors.
Added tests to cover functionality.
Fixed prefix to be serverless/{serviceName}/{stage}
@DavidTanner DavidTanner reopened this Sep 22, 2016
@DavidTanner
Copy link
Author

This should be ready to merge.

@eahefnawy
Copy link
Member

@DavidTanner does this also support creating a bucket based on a custom name? or the bucket must exist?

@DavidTanner
Copy link
Author

@eahefnawy If you want to override the current process of creating a new bucket, then just add the relevant section to the serverless.yml file. This PR expects the bucket to already exist, and be in the same region as the Lambda function.

@eahefnawy eahefnawy self-assigned this Sep 27, 2016
@@ -112,6 +112,17 @@ class AwsCompileFunctions {
throw new Error(`No artifact path is set for function: ${functionName}`);
}

if (this.serverless.service.provider.deploymentBucket) {
Copy link
Member

Choose a reason for hiding this comment

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

could you please add some validation logic for the bucket name along with some friendly error messages? 😊

Copy link
Author

Choose a reason for hiding this comment

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

I added a function to aws/lib/validate that can validate an S3 bucket name.

@@ -112,6 +112,17 @@ class AwsCompileFunctions {
throw new Error(`No artifact path is set for function: ${functionName}`);
}

if (this.serverless.service.provider.deploymentBucket) {
const deploymentBucket = this.serverless.service.provider.deploymentBucket;
Copy link
Member

Choose a reason for hiding this comment

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

also we should use the getServerlessDeploymentBucketName() you updated here imo to make use of the region validation logic.

Copy link
Author

Choose a reason for hiding this comment

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

Validation of the bucket name has been moved to configureStack. That puts the CloudFormation setup in one place.

@dougmoscrop
Copy link
Contributor

What about using

bucket: as the key, and then it's either a string (the name, as this works now)
or an object: { name: String, create: bool }

so you could do

bucket: com.bucket.deploy

This would match the existing behaviour but with a name override and should be shorthand for:

bucket:
    name: com.deploy.bucket
    create: true

but you could override create to false. which would expect the bucket to exist.

The stack is now set up in one place.
The S3 bucket isn't created on create if a bucket is already specified.
Working on configurability for the developer.
There is still the possibility of failing to deploy the stack
if the iamRoleArn is set, and a deploymentBucket is specified
@flomotlik
Copy link
Contributor

@dougmoscrop I don't think we should create the bucket. This is already a more advanced config for people who know which bucket to use in which region. Adding an SDK call to create the bucket just makes the code much more complex and I think is unnecessary for this functionality, because if users want us to create a bucket they can simply use the CF bucket name.

Copy link
Member

@eahefnawy eahefnawy left a comment

Choose a reason for hiding this comment

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

Awesome PR! it doesn't just add custom bucket functionality, it also does some awesome refactoring that we should reflect in our other compile code as well.

PR is generally "Approved" from my side (except for a tiny typo), but I'm still testing it manually for a while. Will leave a G2M message once I'm done.

Thanks tons @DavidTanner 💃

});

it('should reject names that are to short', () =>
Copy link
Member

Choose a reason for hiding this comment

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

*too short

Copy link
Author

Choose a reason for hiding this comment

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

Done. Also fixed too long.

}

if (this.bucketName) {
newFunction.Properties.Code.S3Bucket = this.bucketName;
Copy link
Member

Choose a reason for hiding this comment

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

@DavidTanner the PR is not working for me at all, I keep getting the following CF error:

  Serverless Error ---------------------------------------

     Template format error: Unresolved resource dependencies
     [ServerlessDeploymentBucket] in the Resources block
     of the template

After investigating it seems that this.bucketName is trying to reference this.bucketName in the configureStack.js file, which uses completely different context. So CF knows that there's a custom bucket, but compile function always tries to use the default bucket name, hence the unresolved CF dependency error.

Could you look into that? 😊 ... this is the config I used fyi:

service: custom-bucket-serverless

provider:
  name: aws
  region: us-west-2
  deploymentBucket: custom-bucket-serverless-deployment-2

functions:
  hello:
    handler: handler.hello

Copy link
Author

Choose a reason for hiding this comment

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

I configured it like the artifactDirectoryName, so it is now attached to service.package.deploymentBucket.

Copy link
Author

Choose a reason for hiding this comment

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

Also, I had forgotten to run the deployment locally after the last changes. I ran them again now and everything looks as I was expecting it.

…e instead.

Also added the content-type to the json file so you can read the file in the browser.
Copy link
Member

@eahefnawy eahefnawy left a comment

Choose a reason for hiding this comment

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

@DavidTanner I tested it out after your recent fixes. It's working beautifully now! 😊 ... It's G2M from my side once the conflicts are resolved cc/ @flomotlik

There was a merge issue with createStack
@DavidTanner
Copy link
Author

@eahefnawy @flomotlik Master is merged in and everything still works.

@eahefnawy eahefnawy merged commit 34ff433 into serverless:master Oct 3, 2016
@eahefnawy
Copy link
Member

Thank you kind sir 😊 ... merged 🎉

@DavidTanner DavidTanner deleted the deploy branch October 3, 2016 18:16
@pmuens pmuens added this to the v1.0-ideas milestone Oct 4, 2016
@fakewaffle
Copy link

@eahefnawy, Do you anticipate this being released with 1.0?

@jaydp17
Copy link

jaydp17 commented Nov 16, 2016

Is this available somewhere in the documentation?

@tommedema
Copy link

this feature seems to be undocumented

@ezeql
Copy link

ezeql commented Dec 5, 2017

Please add proper documentation. Thank you

@ethanherbertson
Copy link

@tommedema @ezeql I think it is documented here:

You can specify your own S3 bucket which should be used to store all the deployment artifacts. The deploymentBucket config which is nested under provider lets you e.g. set the name or the serverSideEncryption method for this bucket

https://serverless.com/framework/docs/providers/aws/guide/deploying#tips

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

Successfully merging this pull request may close these issues.

None yet

10 participants