-
Notifications
You must be signed in to change notification settings - Fork 7
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
all: adding codeowners #2290
base: main
Are you sure you want to change the base?
all: adding codeowners #2290
Conversation
.github/CODEOWNERS
Outdated
@@ -0,0 +1,233 @@ | |||
/.github/ @elastisys/goto-pipeline/qa/release | |||
|
|||
/bin/ @scripts-goto |
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.
Shouldnt this be @elastisys/goto-scripts ?
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 seems there are a lot of errors in the CODEOWNERS file, but I imagine it is something sysadmin needs to fix? Specifically, teams not having write access to the repo/not being publicly visible, and team names with /
in them look like they don't work correctly.
.github/CODEOWNERS
Outdated
/helmfile.d/values/prometheus-user-alerts-wc.yaml.gotmpl @elastisys/goto-monitoring-stack | ||
/helmfile.d/values/s3-exporter.yaml.gotmpl @elastisys/goto-monitoring-stack | ||
/helmfile.d/values/sc-servicemonitor.yaml.gotmpl @elastisys/goto-monitoring-stack | ||
/helmfile.d/values/tekton.gotmpl @elastisys/goto-monitoring-stack |
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.
Should be continuous delivery goto?
.github/CODEOWNERS
Outdated
/helmfile.d/stacks/system.yaml @elastisys/goto-security # node-local-dns here as well | ||
/helmfile.d/stacks/tekton.yaml @elastisys/goto-continous-delivery | ||
/helmfile.d/stacks/thanos.yaml @elastisys/monitoring-stack | ||
/helmfile.d/stacks/velero.yaml @ elastisys/security-goto |
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.
/helmfile.d/stacks/velero.yaml @ elastisys/security-goto | |
/helmfile.d/stacks/velero.yaml @elastisys/goto-security |
.github/CODEOWNERS
Outdated
# TODO Refine per dashboard later | ||
/helmfile.d/charts/grafana-dashboards/ @elastisys/goto-monitoring-stack | ||
/helmfile.d/charts/grafana-label-enforcer/ @elastisys/goto-monitoring-stack | ||
/helmfile.d/charts/habor/ @elastisys/goto-container-registry |
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.
/helmfile.d/charts/habor/ @elastisys/goto-container-registry | |
/helmfile.d/charts/harbor/ @elastisys/goto-container-registry |
/helmfile.d/charts/prometheus-servicemonitor/ @elastisys/goto-monitoring-stack | ||
/helmfile.d/charts/rclone/ @elastisys/goto-security | ||
/helmfile.d/charts/s3-exporter/ @elastisys/goto-monitoring-stack | ||
/helmfile.d/charts/tekton-pipelines/ @elastisys/goto-continous-delivery |
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.
/helmfile.d/charts/tekton-pipelines/ @elastisys/goto-continous-delivery | |
/helmfile.d/charts/tekton-pipelines/ @elastisys/goto-continuous-delivery |
You spelled it the way the team is spelled so it was technically correct, but I'd love to get this typo fixed in the team name. @Xartos @robinelastisys
# / | ||
# | ||
|
||
/.gitignore @elastisys/goto-pipeline/qa/release |
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.
What's the motivation for this one?
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 added that goto for .github/
and then thought that this was an adjacent file. But the reasoning was not very concrete 😅
If you want to remove or change it then I'm very open for suggestions.
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 it doesn't really apply to any goto area, probably makes most sense to remove it
.github/CODEOWNERS
Outdated
/helmfile.d/stacks/rclone.yaml @elastisys/goto-security | ||
/helmfile.d/stacks/system.yaml @elastisys/goto-security # node-local-dns here as well | ||
/helmfile.d/stacks/tekton.yaml @elastisys/goto-continous-delivery | ||
/helmfile.d/stacks/thanos.yaml @elastisys/monitoring-stack |
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.
/helmfile.d/stacks/thanos.yaml @elastisys/monitoring-stack | |
/helmfile.d/stacks/thanos.yaml @elastisys/goto-monitoring-stack |
.github/CODEOWNERS
Outdated
/helmfile.d/stacks/dex.yaml @elastisys/goto-ingress/auth | ||
/helmfile.d/stacks/external-dns.yaml @elastisys/goto-ingress/auth | ||
/helmfile.d/stacks/falco.yaml @elastisys/goto-security | ||
/helmfile.d/stacks/fluentd.yaml @elatisys/logging-stack-goto |
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.
/helmfile.d/stacks/fluentd.yaml @elatisys/logging-stack-goto | |
/helmfile.d/stacks/fluentd.yaml @elatisys/goto-logging-stack |
@@ -0,0 +1,233 @@ | |||
/.github/ @elastisys/goto-pipeline/qa/release |
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 doesn't seem to be allowed. Or is it just a bad parser?
Also, do we want to have sub teams with this instead of having it as a parent group? Then the goto-pipeline would automatically include all in all the subteams
.github/CODEOWNERS
Outdated
@@ -0,0 +1,233 @@ | |||
/.github/ @elastisys/goto-pipeline/qa/release | |||
|
|||
/bin/ @scripts-goto |
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.
/bin/ @scripts-goto | |
/bin/ @elastisys/goto-scripts |
|
||
/changelog/ @elastisys/goto-pipeline/qa/release | ||
|
||
/completion/ @elastisys/goto-scripts |
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.
These doesn't seem to be public enough 🤔
/scripts/report/ @elastisys/goto-security | ||
/scripts/requirements/ @elastisys/goto-scripts | ||
/scripts/restore/ @elastisys/goto-container-registry | ||
/scripts/S3/ @elastisys/goto-scripts |
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.
Do we need to have this if we have them for /scripts/*
?
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.
Yes, as far as I understand. This is from the codeowner documentation:
The
docs/*
pattern will match files like
docs/getting-started.md
but not further nested files like
docs/build-app/troubleshooting.md
.
docs/* [email protected]
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.
The documentation says it follows .gitignore
syntax, and the .gitignore
pattern format documentation states:
A trailing "
/**
" matches everything inside. For example, "abc/**
"
matches all files inside directory "abc
", relative to the location of
the.gitignore
file, with infinite depth.
So in theory this should do (replacing all the rules with the prefix /scripts/
):
/scripts/S3/ @elastisys/goto-scripts | |
/scripts/** @elastisys/goto-scripts |
/scripts/restore/ @elastisys/goto-container-registry | ||
/scripts/S3/ @elastisys/goto-scripts | ||
/scripts/sbom/ @elastisys/goto-pipeline/qa/release | ||
/scripts/* @elastisys/goto-scripts |
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.
Could this be changed to
/scripts/* @elastisys/goto-scripts | |
/scripts/ @elastisys/goto-scripts |
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 will give a bit of a different meaning, check my comment to your previous comment.
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.
You should be able to set us for /scripts/
before other entries in script, and the more specific ones will override the generic one.
From the docs:
# In this example, @octocat owns any file in the `/apps`
# directory in the root of your repository except for the `/apps/github`
# subdirectory, as this subdirectory has its own owner @doctocat
/apps/ @octocat
/apps/github @doctocat
/scripts/migration/ @elastisys/goto-pipeline/qa/release | ||
/scripts/report/ @elastisys/goto-security | ||
/scripts/requirements/ @elastisys/goto-scripts | ||
/scripts/restore/ @elastisys/goto-container-registry |
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.
We should probably rename this folder, or create a subfolder
/scripts/restore/ @elastisys/goto-container-registry | |
/scripts/restore/harbor/ @elastisys/goto-container-registry |
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.
Yeah I was thinking about that as well. I was just not sure if I wanted to do that in this PR and if that had implications on other things as well.
As people have noted there seem to be issues with the goto teams not being publicly visible and not being able to have @Xartos or @robinelastisys |
/helmfile.d/charts/cluster-admin-rbac/ @elastisys/goto-security | ||
/helmfile.d/charts/external-dns-endpoints/ @elastisys/goto-ingress/auth | ||
/helmfile.d/charts/external-dns-secrets/ @elastisys/goto-ingress/auth | ||
/helmfile.d/charts/falco-psp-rbac/ @elastisys/goto-security |
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.
Is this chart even used nowadays? 🤔
/scripts/grafana-dashboards @elastisys/goto-monitoring-stack | ||
/scripts/local-clusters/ @elastisys/goto-scripts | ||
/scripts/migration/ @elastisys/goto-pipeline/qa/release | ||
/scripts/report/ @elastisys/goto-security |
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.
/scripts/report/ @elastisys/goto-security | |
/scripts/report/ @elastisys/goto-pipeline-qa-release |
As far as I can see they are as public as they can be 🤔 @robinelastisys Do you know some other way of making them even more public |
Maybe:
|
Yea, that's probably correct. But that feels like a bunch of work that I don't want to manage manually. I'll take a look if this can be managed in a better way. We will probably want to manage the teams in a better way anyway so I think that is a good opportunity to fix that |
# DEVELOPMENT.md | ||
# LICENSE | ||
# README.md | ||
# REQUIREMENTS |
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 should be owned by goto-scripts
IMO as it declares the dependencies for running the scripts in /bin
.
|
||
# | ||
# Skipped files in / | ||
# .editorconfig |
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.
QA?
# LICENSE | ||
# README.md | ||
# REQUIREMENTS | ||
# SECURITY.md |
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.
goto-security?
# .editorconfig | ||
# DEVELOPMENT.md | ||
# LICENSE | ||
# README.md |
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 wonder if this should be maintained by public docs?
Warning
This is a public repository, ensure not to disclose:
What kind of PR is this?
Required: Mark one of the following that is applicable:
Optional: Mark one or more of the following that are applicable:
Important
Breaking changes should be marked
kind/admin-change
orkind/dev-change
depending on typeCritical security fixes should be marked with
kind/security
What does this PR do / why do we need this PR?
This is a first draft of adding codeowners to this repo.
There are probably some things that could be changed or something that was missed. Please at least look at your goto areas to see that they seem sensible. We would also appreciate comments on the skipped files to see if they should be given to any area.
Information to reviewers
Checklist
NetworkPolicy Dashboard