-
Notifications
You must be signed in to change notification settings - Fork 4
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
Create initial provider #1
Conversation
Ready for another look! |
serverless/resource_deployment.go
Outdated
return config.Service, err | ||
} | ||
|
||
func hashServerlessDir(configDir string, packagePath string, configJson []byte) (string, error) { |
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.
Can we get some comments as to what we're hashing, why, and what scenarios we're trying to cache / skip over?
serverless/resource_deployment.go
Outdated
}, | ||
"serverless_bin_dir": &schema.Schema{ | ||
Type: schema.TypeString, | ||
Required: 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.
Do we want to make this not required and instead default to CWD?
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.
My gut is that would be more error-prone, as I haven't seen a project share Terraform modules and serverless.yml
/node_modules
in the same CWD before.
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.
Oh you know what, might be nice to default it to the node_modules
in the config_dir
!
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.
Made the above the default!
Looking great so far! In addition to my current review pass comments, can we maybe get a
Here's https://github.com/FormidableLabs/terraform-aws-serverless/blob/master/CONTRIBUTING.md for reference which has some of what you may want if that's useful! |
Should be ready for another pass! |
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.
Everything outside of the go code (which I'm not familiar with) looks good!
I think we do need a README.md
with full integration instructions as this is a departure from normal SLS workflows and such, particularly for the packaging part if you have to somehow do that outside terraform or can do this with additional config within this provider because that's likely going to be a large stumbling block for our users.
Added README! |
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.
Only skimmed the Go, but lgtm
Creates the initial
serverless_deployment
resource which wraps the Serverless CLI. Only runssls deploy
when the contents ofserverless.yml
and the package ZIP archive contents change. Assumes that users run packaging out-of-band.Hoping to split docs and niceties into another PR while we dogfood the provider.
Example:
To test, you need access to the Formidable account set up with
aws-vault
. Runaws-vault exec formidable
thenmake testacc
.@dmmulroy I'd love your input here since you have some experience with Go!