ci: add owners awareness to deployment-notifier #33968
Conversation
jhchabran
added
dx
| } | ||
| for _, path := range paths { | ||
| fs := codenotify.NewGitFS(g.clonePath, ref) | ||
| ownersList, err := codenotify.Subscribers(fs, path, "CODENOTIFY") |
There was a problem hiding this comment.
temp thing, because there aren't much OWNERS files, so this makes testing easier.
| {{- if .Owners }} | ||
| - Owners: {{- range .Owners}}{{ . }}{{" "}}{{- end}} | ||
| {{- end }} |
There was a problem hiding this comment.
This makes me nervous because there is no opt-out, and with staggered deploys this can be several pings a day
There was a problem hiding this comment.
@bobheadxi What do you think about adding in the handbook's data the following additional fields for each team member?
- Corgi MacDog
github: "woofwoof"
notifications:
deployments:
preprod:
author: true
codeowners: false
cloud:
author: true
codeowners: trueWith the following defaults, that could be overridden by the GitHub labels on PRs?
notifications:
deployments:
preprod:
author: false
codeowners: false
cloud:
author: true
codeowners: true
Side note: presently, there are very few codeowners file in the repo and the team aliases do not ping, therefore they would ping even less.
There was a problem hiding this comment.
That might overload the intent of team.yaml 🤔
There was a problem hiding this comment.
For now, can we only notify OWNERS if we have notify-on-deploy set? That would make the opt-in explicit
There was a problem hiding this comment.
I agree, but right now I don't see an immediate solution other than this :/ Maybe another file possibly?
There was a problem hiding this comment.
For now, can we only notify OWNERS if we have notify-on-deploy set? That would make the opt-in explicit
This would defeat the purpose of giving more visibility to what happens in Cloud, that was mentioned over Slack (https://sourcegraph.slack.com/archives/C89KCDK5J/p1649869394862269).
If we were to decide to revert the changes to the team.yml that's a rather easy fix though. And if we start to see many PRs dropping the notifications, that would also be a strong factual signal that we're too noisy as well.
I also want to run a spike on guessing how a given PR changed the services, that would make the signal VS noise much higher and notifications would be much more useful.
There was a problem hiding this comment.
Let's try this then, and see how it goes. I wonder if that means we should revert notify-on-deploy as well, as well as the service-based notifications
There was a problem hiding this comment.
or introduce the team.yaml field you propose like so:
notifications:
deployments:
opt_in: true
Adds an additional flag to
deployment-notifierto compute the owners for each PR and mentions them posting on Slack.I haven't touched yet on how to handle team notifications, it's something that the teammateResolver can't handle for now.
Depends on: sourcegraph/codenotify#17
Closes https://github.com/sourcegraph/sourcegraph/issues/33923
Test plan