-
-
Notifications
You must be signed in to change notification settings - Fork 317
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
Move almost everything out of autospotting.go #421
base: master
Are you sure you want to change the base?
Conversation
This moves most code from autospotting.go to main.go and adds more tests for the latter. See LeanerCloud#406
@@ -142,3 +116,56 @@ func getRegions(ec2conn ec2iface.EC2API) ([]string, error) { | |||
} | |||
return output, nil | |||
} | |||
|
|||
// handler returns an AWS Lambda handler given a config. | |||
func handler(conf *Config) func(context.Context, json.RawMessage) (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.
Function handler
has a Cognitive Complexity of 21 (exceeds 20 allowed). Consider refactoring.
@cristim My hope was to get as much moved out from I'm open to suggestions for how to increase test coverage further. I see some old discussion on #172, but curious to hear your current thoughts. |
Thanks for moving this forward. I think at some point after merging a number of harmless/easy hunks we'll end up with a large chunk that we'll have to merge at once. I think I prefer to have a larger jump with regressions that we then address quickly instead of an extended period of maybe more subtle regressions. |
Code Climate has analyzed commit 257e09b and detected 1 issue on this pull request. Here's the issue category breakdown:
View more on Code Climate. |
@gabegorelick I just merged the event-based PR a few days ago, would this PR still be relevant if we can rebase it on top of the current code or can we close it? |
@gabegorelick are you still interested in contributing this? There were lots of code changes since this was submitted so perhaps some of this doesn't make sense anymore. |
Sorry, I probably won't have time. |
No worries @gabegorelick, I'll look into it within the next few weeks and try to port to the current code what still makes sense from this work. |
This moves most code from autospotting.go to main.go and adds more tests for the latter.
See #406
Issue Type
Summary
Move code out of the
main
package and add some tests.Code contribution checklist
code under any software license.
to it.
guidelines.
test coverage doesn't decrease.
make full-test
.variables which are also passed as parameters to the
CloudFormation
and
Terraform
stacks defined as infrastructure code.
support per-group overrides using tags.
configurations.
proven to work using log output from various test runs.
appropriate) and formatted consistently to the existing log output.
new configuration options for both stack parameters and tag overrides.
contribution actually resolves it.