-
Notifications
You must be signed in to change notification settings - Fork 652
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
fix(notification): Add application to githubStatus notification context #1106
base: master
Are you sure you want to change the base?
fix(notification): Add application to githubStatus notification context #1106
Conversation
6d1aae7
to
add13f6
Compare
@@ -91,7 +93,7 @@ public void sendNotifications( | |||
content.getStageIndex()); | |||
} else if (config.get("type").equals("pipeline")) { | |||
description = String.format("Pipeline '%s' is %s", content.getPipeline(), status); | |||
context = String.format("pipeline/%s", content.getPipeline()); | |||
context = String.format("%s/pipeline/%s", application, content.getPipeline()); |
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 think this will be quite disruptive for existing users of the feature - if the existing context has been set to required in the github branch-protection settings, then this will leave github waiting for statuses that never match the required value. I think that if the change is made at the end instead, then github will still match it, as a prefix. i.e. change it to pipeline/$pipeline/$application
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.
Thanks for the feedback!
Yeah I did think that any repositories using this feature may be required to update their GitHub checks if they are set to required.
I couldn't seem to find anything online in the GitHub docs that mention a partial match working for the GitHub status. Will do some testing on this today.
Alternatively we could have a user specified context that is sent although this will require more code changes to implement, but would be backwards compatible, or we could look to use a feature flag to specify the full path for GitHub notification context
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.
After some testing it doesn't look like the GitHub status will behave that way
https://docs.github.com/en/rest/reference/repos#statuses
We could add this to the release notes as a potential breaking change to be aware of?
Two alternate ways forward:
- Put this functionality behind some kind of feature flag for echo
detailedGitHubStatusNotification
- Allow users to specify a custom context in the notification pipeline config/deck UI
6f3f9ea
to
632da97
Compare
632da97
to
1acd51f
Compare
3655f00
to
a4a5123
Compare
a4a5123
to
d143845
Compare
Add the application to the github status notification. This helps ensure checks are unique per service when working in a monorepo.
This functionality is behind the feature flag
hasApplicationNameInContext
to ensure backwards compatibility.Issue: spinnaker/spinnaker#6442