Add GitHub labels tooling #1466
Conversation
Labels database file formatThe labels database is a YAML template file. See the Label SummaryThe information below was generated by: $ kata-github-labels show categories --with-labels cmd/github-labels/labels.yaml.inNote: This information is presented in a couple of other formats in sections below. Label CategoriesThe table below was generated by: $ kata-github-labels show categories --with-labels --format md cmd/github-labels/labels.yaml.in
LabelsThe table below was generated by: $ kata-github-labels show labels --format md cmd/github-labels/labels.yaml.in
|
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
9063fd8 to
ed04688
Compare
grahamwhaley
left a comment
grahamwhaley
left a comment
There was a problem hiding this comment.
minor feedback, nothing major.
looking good, although I do have 'label shock', and may need a lay down....
.ci/static-checks.sh
Outdated
| cidir=$(dirname "$0") | ||
| source "${cidir}/lib.sh" | ||
|
|
||
| tests_dir="${GOPATH}/src/github.com/kata-containers/tests" |
There was a problem hiding this comment.
I see this is not a change in functionality, but I wonder if the path should be relative to say ${cidir}, rather than hard wired to the gopath github repo. Hard wiring can make it hard to test changes if you happen to have a different local layout etc. (I have run into such things before, which is where a number of this sort of idioms we have in a lot of scripts came from:
export tests_repo="${tests_repo:-github.com/kata-containers/tests}"
export tests_repo_dir="$GOPATH/src/$tests_repo"
cmd/github-labels/Makefile
Outdated
| # | ||
|
|
||
| TARGET = kata-github-labels | ||
| SOURCES = $(shell find . 2>&1 | grep -E '.*\.go$$') |
There was a problem hiding this comment.
heh, maybe add a -type f to the find, just in case somebody has a subdir called stuffyou.go or something :-) heh. I know how paranoid you are.....
/me winces at find in a Makefile.... shudder......
There was a problem hiding this comment.
... ftr, that Makefile is similar / almost identical to a bunch of other makefiles in our repos. PRs welcome! ;)
cmd/github-labels/README.md
Outdated
| The script validates the combined labels database by performing a number of | ||
| checks including running the `kata-github-labels` tool (described in the | ||
| [Checking and summarising the labels database](#checking-and-summarising-the-labels-database) section) | ||
| in its checking mode. |
cmd/github-labels/github-labels.go
Outdated
| githubTableSeparator, | ||
| "URL", | ||
| githubTableSeparator) | ||
| fmt.Printf("|-|-|-|\n") |
There was a problem hiding this comment.
Don't know if it will help or not, but for the checkmetrics tool table results I used https://github.com/olekukonko/tablewriter#example-5---markdown-format - and it looks like it can do markdown output with some tweaks.
There was a problem hiding this comment.
It might be overkill tbh, but I'll take a look...
cmd/github-labels/github-labels.sh
Outdated
|
|
||
| yq read "$file" >/dev/null | ||
|
|
||
| yamllint --strict "$file" |
There was a problem hiding this comment.
above we check if we have yq installed - should we do similar (but maybe have to abort) if we don't find yamllint?
There was a problem hiding this comment.
Fixed - I want to keep this PR as minimal as possible so it just dies if yamllint isn't installed for now - it will be installed on the CI systems.
cmd/github-labels/labels.yaml.in
Outdated
| Containers GitHub repositories. | ||
|
|
||
| Each repository can optionally contain a top-level `labels.yaml` that | ||
| specifies a list of repository-specific labels. This labels in the |
There was a problem hiding this comment.
s/This labels/The labels/
cmd/github-labels/labels.yaml.in
Outdated
| A label may also specify a colour definition. | ||
|
|
||
| If a label specifies a category, it must be a category from the list of | ||
| categories specified in this file. |
There was a problem hiding this comment.
Can a repo define its own categories as well?
| - name: wrong-repo | ||
| description: Raised in incorrect repository | ||
| category: resolution | ||
| color: DEFAULT_COLOUR |
There was a problem hiding this comment.
I think my eyes have label blur 😱 That's a lot of categories and labels. OK, so, looking at some of the defacto-ish projects that have formalised their labels (k8s, rkt, docker), it is not a dissimilar list or size - but kudos to the first (or will that be 2nd) person who reads and digests the whole list ;-)
There was a problem hiding this comment.
Feel free to comment on particular labels. I'm sure there are issues and we can argue about individual labels or categories on the PR.
Oh, and spare a thought for the poor author who had to put this whole thing together... and yes, it all took a lot of my time ;)
.ci/static-checks.sh
Outdated
|
|
||
| info "Checking labels for repo ${repo} using temporary combined database ${tmp}" | ||
|
|
||
| bash -f "${tests_dir}/cmd/github-labels/github-labels.sh" "generate" "${repo}" "${tmp}" |
There was a problem hiding this comment.
nitpick - the script is set -e, so if this fails, then we'll leave the tmpfile around. We could trap hook its deletion. picky, I know.
There was a problem hiding this comment.
That's by design - the info call specifically mentions the temporary file so that if the script fails, you can inspect it to see wtf caused the problem. The file is tiny so I don't think we'll ENOSPC the CI systems ;)
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
4 times, most recently
from
b939875 to
6918e6c
Compare
|
I've regenerated #1466 (comment) using the new CLI syntax. @egernst, @kata-containers/architecture-committee, @mimeriza - ptal. |
|
/retest |
|
@grahamwhaley - I'm now using |
|
@klynnrif - ptal at the new |
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
6918e6c to
f05058e
Compare
|
/retest |
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
f05058e to
a485bc4
Compare
|
/retest |
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
a485bc4 to
9a58efc
Compare
|
/retest |
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
2 times, most recently
from
4ff154a to
70f32c0
Compare
|
sigh... /retest |
.ci/static-checks.sh
Outdated
|
|
||
| check_labels() | ||
| { | ||
| [ $(uname -s) != Linux ] && info "Cannot only check labels under Linux" && return |
cmd/github-labels/check.go
Outdated
| return fmt.Errorf("invalid category %v found for label %+v", catName, l) | ||
| } | ||
|
|
||
| // update |
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
2 times, most recently
from
52937ff to
dc322d3
Compare
|
/tetest |
|
/retest even! :) |
klynnrif
left a comment
klynnrif
left a comment
There was a problem hiding this comment.
Scrubbed for grammar, structure, and flow. A few suggested rewrites. Thanks!
cmd/github-labels/README.md
Outdated
| The Kata Project uses a number of GitHub repositories. To allow issues and PRs | ||
| to be handled consistently between repositories a standard set of issue labels | ||
| are used. These labels are stored in YAML format in the master [labels | ||
| database template](labels.yaml.in). This file is human-readable and |
There was a problem hiding this comment.
Lines 14-16 suggested rewrite:
... This file is human-readable, machine-readable, and self-describing (see the file for the introductory description).
cmd/github-labels/README.md
Outdated
| machine-readable and self-describing (take a look at the file for the | ||
| introductory description). | ||
|
|
||
| Additionally, each repository can contain a set of additional |
There was a problem hiding this comment.
Lines 18-19 suggested rewrite:
Each repository can contain a set of additional (repository-specific) labels, which are stored in a top-level YAML template...
cmd/github-labels/README.md
Outdated
|
|
||
| # Generating the combined labels database | ||
|
|
||
| The combined labels database is generated by running the |
There was a problem hiding this comment.
Lines 27-30 suggested rewrite (unless I changed the meaning):
You can run the github_labels.sh script with the generate parameter to generate the combined labels database. This passes the repository in order to generate the combined labels database and the name of a file to write the combined database:
There was a problem hiding this comment.
I've changed your wording slightly to:
You can run the
github_labels.shscript with thegenerateargument to
create the combined labels database. The additional arguments specify the
repository (in order to generate the combined labels database) and the name of
a file to write the combined database:
cmd/github-labels/README.md
Outdated
| $ ./github-labels.sh generate github.com/kata-containers/runtime /tmp/combined.yaml | ||
| ``` | ||
|
|
||
| The script validates the combined labels database by performing a number of |
There was a problem hiding this comment.
Lines 36-39 suggested rewrite:
This script validates the combined labels database by performing a number of checks, including running the kata-github-labels tool in checking mode. See the Checking and summarising the labels database section for more information.
cmd/github-labels/README.md
Outdated
|
|
||
| # Checking and summarising the labels database | ||
|
|
||
| The [`kata-github-labels`](github-labels.go) tool is used to check and |
There was a problem hiding this comment.
Lines 43-44 suggested rewrite:
The kata-github-labels tool checks and summarizes the labels database for each repository.
cmd/github-labels/README.md
Outdated
| ``` | ||
| ## Check only | ||
|
|
||
| To only perform checks on specified labels database: |
There was a problem hiding this comment.
Line 63 suggested rewrite:
Performs checks on a specified labels database:
OR
Performs checks on specified labels databases:
cmd/github-labels/README.md
Outdated
|
|
||
| ## Full details | ||
|
|
||
| To list all available options: |
There was a problem hiding this comment.
Line 71 suggested rewrite:
Lists all available options:
cmd/github-labels/README.md
Outdated
|
|
||
| ## Show categories | ||
|
|
||
| To show all information about categories: |
There was a problem hiding this comment.
Lines 56 suggested rewrite:
Shows all information about categories:
cmd/github-labels/README.md
Outdated
|
|
||
| ## Show labels | ||
|
|
||
| To display a summary of the labels: |
There was a problem hiding this comment.
Line 48 suggested rewrite:
Displays a summary of the labels:
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
dc322d3 to
9c2ccd5
Compare
|
/test |
|
/retest |
|
|
@egernst I think @jodh-intel noted that the |
.ci/setup_env_sles.sh
Outdated
| chronic sudo -E zypper -n install -t pattern "Basis-Devel" && sudo -E zypper -n install python zlib-devel | ||
|
|
||
| echo "Install YAML validator" | ||
| chronic sudo -E zypper -n install yamllint |
There was a problem hiding this comment.
@jodh-intel that does not exist on sles or opensuse, this is the CI error
'yamllint' not found in package names. Trying capabilities.
There was a problem hiding this comment.
stepping back.... the intention is to test that our label tooling is adequate. There really shouldn't be a need to test this on a per-distro basis -- just in a single job would be enough.
Is there a way to skip this for openSUSE? WDYT?
There was a problem hiding this comment.
now python3-pip not found on SLES:
04:18:37 Package 'python3-pip' not found.
There was a problem hiding this comment.
$ zypper -n install python2-pip
There was a problem hiding this comment.
@egernst - You are correct. At this stage, we're just checking the single db file so this check could be summarised as:
-
Only run this when one of the db files changes.
But in the future, the plan is to add extra checks on the labels on the PR and maybe even its associated issue too. In fact, we may end up adding labels. At that point it would become a:
-
Run exactly once per PR.
That implies of course that it should not run on every CI system.
I think we're still waiting for the ability to run arbitrary periodic jobs under Jenkins but this should be considered as currently it will be running "too many times".
Until we can move it though, does anyone know how to install pip on SLES 12? If I don't find out by Monday, I'll have to disable for SLES which would be a shame as all the distros should have the ability to run all tests in principle.
There was a problem hiding this comment.
$ zypper -n install python2-pip
There was a problem hiding this comment.
Grr - browser cache fail. Branch updated to use python2-pip for sles now.
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
2bd1098 to
34fda58
Compare
|
/retest |
Move a variable to the top-level for a future change. Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
2 times, most recently
from
a5a3dd8 to
cc98c02
Compare
|
/retest |
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
cc98c02 to
fd7cf13
Compare
|
/retest |
Install `yamllint` for the SUSE-based distros. This is already done for all the other distros. Signed-off-by: James O. D. Hunt <[email protected]>
Add new tooling to handle GitHub labels. This includes: - A YAML template file (the master "labels database") This contains the master set of labels to be used for all Kata repositories. All labels have descriptions and a category. - A script to generate the combined set of labels This merges the master set of labels with an optional repository-specific set of labels to produce a combined set of labels for a particular repository. - A golang tool to check and summarise the labels database This can also be used to sort the labels database. Fixes kata-containers#1463. Signed-off-by: James O. D. Hunt <[email protected]>
Correct bad formatting caused by using spaces rather than tabs. Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
fd7cf13 to
57e6ad6
Compare
|
Right. Updated for the bazillionth time for OpenSUSE and SLES. Let's see how this goes... /me gets a 🔫 and a bottle of 🍾 ready to cover all outcomes... |
|
/retest and ... run....! |
|
Travis (OSX only) timed out, so restarted. Restarting to make my eyes stop bleeding... |
|
@jodh-intel I've said it before, I'll say it again. I guess I'll just have to keep saying it ... all the java stack dump stuff is not any of the tests we are running, on any of the CI jobs - it is the jenkins agent way of saying it got miffed at the machine under test going belly up - so, you only need to post the top line of the java dump, and the few pertinent lines before it :-) |
|
@grahamwhaley - I know it's not us, but I'm alerting all to the problem ;) Pasting the entire thing anyhow seems sensible given we do see variation in the java barfs. For example, there was that uber-verbose |
Refactor the check for `yamllint` to allow it to be used by other functions in the future. Signed-off-by: James O. D. Hunt <[email protected]>
Add a `--labels` option to `static-checks.sh` to allow the combined labels database to be checked. Signed-off-by: James O. D. Hunt <[email protected]>
jodh-intel
force-pushed
the
add-github-labels-tooling
branch
from
57e6ad6 to
af421d0
Compare
|
/retest |
|
omg! It's working! 😄 |
|
/me rekicks opensuse after seeing CPU hotplug failure... |
6 participants
Add new tooling to handle GitHub labels. This includes:
A YAML template file (the master "labels database")
This contains the master set of labels to be used for all Kata repositories. All labels have descriptions and a category.
A script to generate the combined set of labels
This merges the master set of labels with an optional repository-specific set of labels to produce a combined set of labels for a particular repository.
A golang tool to check and summarise the labels database
This can also be used to sort the labels database.
Fixes #1463.
Signed-off-by: James O. D. Hunt [email protected]