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

Initial commit for handling of autoscalingEvents #16

Merged
merged 3 commits into from Feb 3, 2018

Conversation

ibrokethecloud
Copy link
Contributor

The repo was missing ability to handle asg lifecycle events.

Added functionality to handle autoscaling lifecycle events and action on them.

Copy link
Collaborator

@bmoffatt bmoffatt left a comment

Choose a reason for hiding this comment

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

Thanks for adding this! Would you also be willing to add the other samples from https://docs.aws.amazon.com/autoscaling/ec2/userguide/cloud-watch-events.html#cloudwatch-event-types to the testdata?

type EventDetails struct {
StatusCode string `json:"StatusCode,omitempty"` //Status code for Event
AutoscalingGroupName string `json:"AutoScalingGroupName,omitempty"` //AutoScalingGroup name
ActivityId string `json:"ActivityId,omitempty"` //ActivityId
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to ActivityID

AutoscalingGroupName string `json:"AutoScalingGroupName,omitempty"` //AutoScalingGroup name
ActivityId string `json:"ActivityId,omitempty"` //ActivityId
Details DetailsInfo `json:"Details,omitempty"` //Additional details about event such as AZ and SubnetId of instance
RequestId string `json:"RequestId,omitempty"` //RequestId of Request
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to RequestID

RequestId string `json:"RequestId,omitempty"` //RequestId of Request
StatusMessage string `json:"StatusMessage,omitempty"` //StatusMEssage
EndTime time.Time `json:"EndTime,omitempty"` //EndTime of Event
EC2InstanceId string `json:"EC2InstanceId,omitempty"` //InstanceID of ec2 instance impacted by event
Copy link
Collaborator

Choose a reason for hiding this comment

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

change to EC2InstanceID

}

// EventDetails struct is used to parse the nested event Details available within the Autoscaling event
type EventDetails struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

to avoid polluting the package with many simple type names, change this to something like AutoScalingEventDetail

}

// DetailsInfo contains AZ and SubnetId for instance impacted by the autoscaling event //
type DetailsInfo struct {
Copy link
Collaborator

Choose a reason for hiding this comment

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

to avoid polluting the package with many simple type names, change this to something like AutoScalingEventDetailDetails

… them during encoding when the fields are empty.
@ibrokethecloud
Copy link
Contributor Author

Updated the code to have additional test cases for all samples available in the product documentation.

Have changed certain fields for AutoScalingEvents to pointer types to allow encoding to effectively handle empty fields which helps with the test cases.

@bmoffatt
Copy link
Collaborator

Thanks for adding the other event samples.

After looking at the samples again, this approach creates a weird user experience. The Details member being the union all optional fields of all the different AutoScaling events means that a user should check against the empty string or nil before every use.

What do you think of having a separate AutoScaling event type for each of the samples in the documents? The alternative would be to only model the generic CloudWatchEvent with the Detail field as a map[string]interface{}

@ibrokethecloud
Copy link
Contributor Author

I am in favour of converinting the Detail field into a map[string]interface{} and adding additional event typed Detail structures to allow parsing the results based on requirements.

I will make the changes soon and update the request.

… for better handling and parsing of event information
@bmoffatt bmoffatt merged commit 402b36f into aws:master Feb 3, 2018
Yannic referenced this pull request in Yannic/aws-lambda-go Feb 7, 2018
* Initial commit for handling of autoscalingEvents

* Added additional test files. Changed some field to pointers to ignore them during encoding when the fields are empty.

* Updated AutoScalingEvent Details to a map[string]interface{} to allow for better handling and parsing of event information
Yannic referenced this pull request in Yannic/aws-lambda-go Feb 7, 2018
* Initial commit for handling of autoscalingEvents

* Added additional test files. Changed some field to pointers to ignore them during encoding when the fields are empty.

* Updated AutoScalingEvent Details to a map[string]interface{} to allow for better handling and parsing of event information
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants