-
Notifications
You must be signed in to change notification settings - Fork 109
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
Target fixes and change event generation #167
Conversation
f456930
to
b07b6fb
Compare
12f4a19
to
4b9a76d
Compare
|
||
// Message is message for k8s event | ||
func (c CanaryEvent) Message() string { | ||
return fmt.Sprintf("%s - Canary for app %s | version %d - %s", c.Name, c.AppName, c.DeploymentVersion, c.Description) |
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.
I'd probably make "%s - Canary for app %s | version %d - %s"
. Not a huge concern. Same with the NextStepEvents and ChangeEvents below.
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.
Looks good to me.
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.
Looks good! There is a small typo, but it isn't anything that should hold back this pr.
Rename and move AppReconcileOutcome and add tests Improve events Revert unecessary change Fix test Fix tests Add annotations (#176) * Add annotations to canary events * Fix tests, remove unneccessary code * Create consts for annotation names * Remove unneccessary comments and prints
3103217
to
4e176f0
Compare
Description
Create dedicated structs for generated Canary events
Type of change
Testing
Documentation
Final Checklist: