Skip to content
This repository was archived by the owner on Jun 28, 2024. It is now read-only.

CI: Add TODO/FIXME checker#445

Merged
GabyCT merged 1 commit intokata-containers:masterfrom
jodh-intel:add-todo-fixme-finder
Jun 29, 2018
Merged

CI: Add TODO/FIXME checker#445
GabyCT merged 1 commit intokata-containers:masterfrom
jodh-intel:add-todo-fixme-finder

Conversation

@jodh-intel
Copy link
Copy Markdown

@jodh-intel jodh-intel commented Jun 26, 2018

Alert users to TODO/FIXME comments that tend never to get fixed by
failing the CI unless those comments also refer to a github issue
where the problem is being tracked.

Fixes #444.

Signed-off-by: James O. D. Hunt james.o.hunt@intel.com

grahamwhaley
grahamwhaley approved these changes Jun 26, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

.ci/static-checks.sh Outdated

# Tests to apply to all files.
#
# Currently just looks for TODO/FIXME comments that should be removed.
Copy link
Copy Markdown
Contributor

@grahamwhaley grahamwhaley Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/removed/converted to or annotated with an Issue/ ?

Copy link
Copy Markdown
Author

@jodh-intel jodh-intel Jun 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call - fixed.

@grahamwhaley
Copy link
Copy Markdown
Contributor

grahamwhaley commented Jun 26, 2018

Battoning down the hatches eh? I can't dis-agree, but am bracing myself for the flurry of failed PRs that touch a file that already has an errant comment in it...
Out of interest then, have you run this on the current code bases, and what did you find - do we have many current offending comments?

@jodh-intel jodh-intel force-pushed the add-todo-fixme-finder branch from 3f50caf to c80054f Compare June 26, 2018 10:28
CI: Add TODO/FIXME checker
94ae588 
Alert users to TODO/FIXME comments that tend never to get fixed by
failing the CI *unless* those comments also refer to a github issue
where the problem is being tracked.

Fixes kata-containers#444.

Signed-off-by: James O. D. Hunt <james.o.hunt@intel.com>
@jodh-intel jodh-intel force-pushed the add-todo-fixme-finder branch from c80054f to 94ae588 Compare June 26, 2018 10:36
@jodh-intel
Copy link
Copy Markdown
Author

jodh-intel commented Jun 26, 2018 •
edited
Loading

Well, yes. But we need to collectively agree this is a good idea so I'll step back and let folk ack/nack :)

We're actually in pretty good shape. Calling just that new function on all the kata repos from a script that just adds in the name of the repo it's checking, we have the following:

INFO: Checking repo https://github.com/kata-containers/agent
INFO: Checking files
ERROR: The following files contain TODO/FIXME's that need converting to issues:

./grpc_test.go
./Dockerfile
./protocols/grpc/agent.proto
./protocols/grpc/agent.pb.go
./grpc.go

INFO: Checking repo https://github.com/kata-containers/ci
INFO: Checking files
ERROR: The following files contain TODO/FIXME's that need converting to issues:

./VMs/metrics/3_complete_baseimage.sh
./VMs/metrics/global_env.sh

INFO: Checking repo https://github.com/kata-containers/community
INFO: Checking files
INFO: Ignoring comment tag in document ./PR-Review-Guide.md
INFO: Checking repo https://github.com/kata-containers/documentation
INFO: Checking files
INFO: Checking repo https://github.com/kata-containers/ksm-throttler
INFO: Checking files
INFO: Checking repo https://github.com/kata-containers/osbuilder
INFO: Checking files
INFO: Checking repo https://github.com/kata-containers/packaging
INFO: Checking files
ERROR: The following files contain TODO/FIXME's that need converting to issues:

./scripts/configure-hypervisor.sh
./.ci/teardown.sh
./.ci/run.sh

INFO: Checking repo https://github.com/kata-containers/proxy
INFO: Checking files
INFO: Checking repo https://github.com/kata-containers/runtime
INFO: Checking files
ERROR: The following files contain TODO/FIXME's that need converting to issues:

./virtcontainers/container.go
./virtcontainers/qemu.go
./virtcontainers/sandbox.go
./virtcontainers/hyperstart_agent.go
./virtcontainers/kata_builtin_proxy.go
./virtcontainers/proxy.go
./virtcontainers/qemu_arch_base.go
./virtcontainers/network.go
./virtcontainers/kata_agent.go
./virtcontainers/filesystem.go
./virtcontainers/nsenter.go

INFO: Checking repo https://github.com/kata-containers/shim
INFO: Checking files
INFO: Checking repo https://github.com/kata-containers/tests
INFO: Checking files
ERROR: The following files contain TODO/FIXME's that need converting to issues:

./metrics/density/docker_memory_usage.sh
./cmd/checkmetrics/main.go
./cmd/checkmetrics/compare.go
./cmd/checkmetrics/compare_test.go

But that's the total, so users will only need to fix those comments if they happen to change those files.

If we plan to land this, yes, we'll need to go round converting the files listed above though to ensure the `master` build doesn't bomb out ;)

@GabyCT
Copy link
Copy Markdown
Contributor

GabyCT commented Jun 29, 2018 •
edited by amshinde
Loading

lgtm

Approved with PullApprove

@GabyCT GabyCT merged commit 335a034 into kata-containers:master Jun 29, 2018
@jodh-intel jodh-intel mentioned this pull request Jul 3, 2018
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@grahamwhaley grahamwhaley grahamwhaley approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Add TODO/FIXME CI test

3 participants

@jodh-intel @grahamwhaley @GabyCT