tools: add metadata linting step to linter job #1087

dhellmann · 2022-04-10T17:03:33Z

Add a metadata-lint command to the tools directory for checking enhancements metadata.
Update the existing job to run the new tool against enhancements markdown files that are changed in the current pull request.
Require an approver in the metadata.
Require at least one reviewer in the metadata.
Require a string that looks like a URL in the tracking-links metadata.
Update the comments in the metadata section of the template to provide better guidance about the new linter rules.

dhellmann · 2022-04-10T17:04:16Z

/uncc cgwalters
/uncc ststs
/uncc soltysh
/cc @bparees
/cc @aravindhp
/cc @kikisdeliveryservice

Signed-off-by: Doug Hellmann <[email protected]>

guidelines/enhancement_template.md

wking · 2022-04-11T18:28:46Z

tools/enhancements/metadata.go

+	// Must have one valid tracking link and no TBD
+	foundLink := false
+	for _, trackingLink := range m.TrackingLinks {
+		if trackingLink == "TBD" {


We can probably ban TBD in all metadata properties, right? Or perhaps an allow-list of properties where it is a valid value?

We could. I was focusing on the fields that seemed most important right now, based on issues I've seen with PRs.

aravindhp

Thanks for doing this, @dhellmann. Just a few non-blocking comments.

tools/cmd/metadataLint.go

aravindhp · 2022-04-11T17:41:59Z

tools/enhancements/metadata.go

+		if trackingLink == "TBD" {
+			reportError("'TBD' is not a valid value for tracking-link")
+			continue
+		}
+		if trackingLink == "" {
+			reportError("empty string for tracking-link")
+			continue
+		}


I would simplify and throw an error if the tracking link is not an URL.

I was trying to differentiate between a missing value and an invalid value. Do you think people would understand "could not parse tracking link ''" as meaning the value was empty and it can't be?

How about making the error message something like tracking link has to be a valid URL?

tools/enhancements/metadata.go

hack/metadata-lint.sh

tools/enhancements/metadata.go

dhellmann · 2022-04-11T21:25:55Z

tools/enhancements/metadata.go

+		if trackingLink == "TBD" {
+			reportError("'TBD' is not a valid value for tracking-link")
+			continue
+		}
+		if trackingLink == "" {
+			reportError("empty string for tracking-link")
+			continue
+		}


Signed-off-by: Doug Hellmann <[email protected]>

We want a copy of the template to fail the linter until the TBD value is replaced with a real name, so remove the things that look like real names. Signed-off-by: Doug Hellmann <[email protected]>

Signed-off-by: Doug Hellmann <[email protected]>

Move the comments about the reviewers into quotes so the YAML can be parsed. Signed-off-by: Doug Hellmann <[email protected]>

Co-authored-by: Ben Parees <[email protected]>

dhellmann · 2022-04-11T21:48:36Z

I believe I have made updates based on all of the comments on the earlier drafts.

aravindhp

/lgtm

bparees · 2022-04-13T19:58:23Z

/approve

openshift-ci · 2022-04-13T19:58:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bparees

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [bparees]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2022-04-13T20:08:08Z

@dhellmann: all tests passed!

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Catching up with 36e98de (template: update guidance for specifying that no API approver is needed, 2022-04-10, openshift#1087).

openshift-ci bot requested review from jerpeter1 and zaneb April 10, 2022 17:03

openshift-ci bot requested review from aravindhp, bparees and kikisdeliveryservice April 10, 2022 17:04

dhellmann force-pushed the metadata-lint branch from 7f8b2ae to f8f649f Compare April 10, 2022 17:50

tools: only require config file in commands that use the contents

842a0f1

Signed-off-by: Doug Hellmann <[email protected]>

dhellmann force-pushed the metadata-lint branch from f8f649f to 359c865 Compare April 10, 2022 17:52

bparees reviewed Apr 11, 2022

View reviewed changes

guidelines/enhancement_template.md Outdated Show resolved Hide resolved

wking reviewed Apr 11, 2022

View reviewed changes

aravindhp reviewed Apr 11, 2022

View reviewed changes

dhellmann commented Apr 11, 2022

View reviewed changes

dhellmann force-pushed the metadata-lint branch from d2451aa to b3328be Compare April 11, 2022 21:37

dhellmann and others added 6 commits April 11, 2022 17:39

tools: add a metadata-lint command

f012c1c

Signed-off-by: Doug Hellmann <[email protected]>

tools: run metadata-lint as part of the existing linter job

555d198

Signed-off-by: Doug Hellmann <[email protected]>

template: remove bogus values from lists of people

93ade7c

We want a copy of the template to fail the linter until the TBD value is replaced with a real name, so remove the things that look like real names. Signed-off-by: Doug Hellmann <[email protected]>

template: update guidance for specifying that no API approver is needed

36e98de

Signed-off-by: Doug Hellmann <[email protected]>

single-node-openshift-with-workers: fix metadata to pass linter

355b9e2

Move the comments about the reviewers into quotes so the YAML can be parsed. Signed-off-by: Doug Hellmann <[email protected]>

Update guidelines/enhancement_template.md

38ea128

Co-authored-by: Ben Parees <[email protected]>

dhellmann force-pushed the metadata-lint branch from b3328be to 38ea128 Compare April 11, 2022 21:39

aravindhp reviewed Apr 11, 2022

View reviewed changes

openshift-ci bot assigned aravindhp Apr 11, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Apr 11, 2022

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 13, 2022

openshift-merge-robot merged commit 9c1658c into openshift:master Apr 13, 2022

dhellmann mentioned this pull request Apr 14, 2022

enhancements process documentation improvements #1080

Merged

tools: add metadata linting step to linter job #1087

tools: add metadata linting step to linter job #1087

Uh oh!

Conversation

dhellmann commented Apr 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dhellmann commented Apr 10, 2022

Uh oh!

Uh oh!

wking Apr 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dhellmann Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

aravindhp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aravindhp Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

dhellmann Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

aravindhp Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

dhellmann Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dhellmann Apr 11, 2022

Choose a reason for hiding this comment

Uh oh!

dhellmann commented Apr 11, 2022

Uh oh!

aravindhp left a comment

Choose a reason for hiding this comment

Uh oh!

bparees commented Apr 13, 2022

Uh oh!

openshift-ci bot commented Apr 13, 2022

Uh oh!

openshift-ci bot commented Apr 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dhellmann commented Apr 10, 2022 •

edited

Loading

wking Apr 11, 2022 •

edited

Loading