This repository has been archived by the owner on Oct 11, 2019. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
VYGR-391: Add opsgenie integrations to notifications #120
base: master
Are you sure you want to change the base?
VYGR-391: Add opsgenie integrations to notifications #120
Changes from 15 commits
005de8c
f37c6fa
fcb59fa
45259db
dd63115
46e0cfb
b7e6865
cd3ba39
9022baf
0d61496
4844d76
533eb33
ce4b544
9368162
d13b742
c811811
0271bef
41066cb
964933c
e3e587b
434198e
f229991
a5ac769
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 am not sure why this always returned an error and I cannot identify where this function is called.
I think its called in
readAndValidateOptions
whenyaml.UnmarshalStrict
is called, but if thats the case then I'm not sure why we always fail.https://github.com/atlassian/voyager/blob/master/cmd/synchronization/app/options.go#L67
@ash2k Can you help me understand why we returned an error 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.
seems like a bug, not sure how did it work
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.
it should be implicitly called in https://github.com/atlassian/voyager/blob/master/cmd/synchronization/app/options.go#L67
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.
Don't use real data in tests please. Use fake data =)
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.
This is a manual integration test, I don't think fake data is applicable.
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.
cc @jokeyrhyme
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.
?
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.
Why not
global
?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 am not sure why its not called "global". @jokeyrhyme Do you know why?
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.
@fcobb Well, it's not that things are global, it's just that they aren't explicitly environment-specific
Maybe this should actually mean Global, and maybe we should actually emit
"global"
instead of"null"
When we put this together, the most important thing was making sure our CloudWatch alarms (which are environment-specific) were covered, everything else was secondary
I'm wondering whether we should call this Null or something so that we can meaningfully transition to Global in future?
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.
@jokeyrhyme I went for Global as if its not environment specific then its essentially global.
As you own this API I will defer these decisions to you and your team. My preference is to return a specific value for these non-environment specific values rather than null.
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.
Previously, we filtered the service attributes before adding them to the service object. We now do that filtering later on.