Skip to content
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

API linter framework and namesMatch API rule #65737

Merged
merged 5 commits into from Jul 14, 2018

Conversation

@roycaihw
Copy link
Member

roycaihw commented Jul 3, 2018

What this PR does / why we need it:
Bump kube-openapi dependency to use the API linter framework in k/k OpenAPI spec generation procedure.

Currently one API rule is enforced:
"Go field names must be CamelCase. JSON field names must be camelCase. Other than capitalization of the initial letter, the two should almost always match. No underscores nor dashes in either."

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #65244

Special notes for your reviewer:
Most code change in this PR was generated (~1700 lines). Please see commits for detail.

Release note:

NONE

/sig api-machinery
/cc @pwittrock @mbohlool

@k8s-ci-robot k8s-ci-robot requested a review from mbohlool Jul 3, 2018

@k8s-ci-robot k8s-ci-robot requested a review from pwittrock Jul 3, 2018

@roycaihw roycaihw force-pushed the roycaihw:api-linter branch 4 times, most recently from 3cbcb82 to a1b9040 Jul 3, 2018

@roycaihw roycaihw referenced this pull request Jul 9, 2018

Closed

API linter #54887

@roycaihw roycaihw force-pushed the roycaihw:api-linter branch from a1b9040 to 391ec5a Jul 11, 2018

roycaihw added some commits Jul 11, 2018

Bump kube-openapi dependency
to pick up changes about API linter framework and namesMatch API rule
Use kube-openapi cmd in Make rules
check in existing API rule violations;
the Make rule fails if generated violation report differs from the
checked-in violation file and prints error message;
add documentation.

@roycaihw roycaihw force-pushed the roycaihw:api-linter branch from 391ec5a to 3af6061 Jul 11, 2018

@roycaihw

This comment has been minimized.

Copy link
Member Author

roycaihw commented Jul 11, 2018

/test pull-kubernetes-e2e-gce-100-performance

@@ -402,6 +402,10 @@ $(CONVERSION_GEN): $(k8s.io/kubernetes/vendor/k8s.io/code-generator/cmd/conversi
OPENAPI_BASENAME := $(GENERATED_FILE_PREFIX)openapi
OPENAPI_FILENAME := $(OPENAPI_BASENAME).go
OPENAPI_OUTPUT_PKG := pkg/generated/openapi
BOILERPLATE_FILENAME := vendor/k8s.io/code-generator/hack/boilerplate.go.txt
REPORT_FILENAME := $(OUT_DIR)/violations.report
KNOWN_VIOLATION_FILENAME := hack/testdata/api-rules/violations.report

This comment has been minimized.

@mbohlool

mbohlool Jul 11, 2018

Member

should this be called `violation_exceptions.list" or something like that? (change list to the format of the file, csv, yaml, etc?)

This comment has been minimized.

@roycaihw

roycaihw Jul 11, 2018

Author Member

Yes it's indeed an exception / whitelist file. I used "violations.report" instead of "whitelist" because I didn't want people to feel that it's okay to add more entries to it.

I updated the README to emphasize that the entries in the list should only be removed but not added. Since the file is not in csv format, I updated the filename to violation_exceptions.list

-h $(BOILERPLATE_FILENAME) \
-r $(REPORT_FILENAME) \
"$$@"; \
diff $(REPORT_FILENAME) $(KNOWN_VIOLATION_FILENAME) || \

This comment has been minimized.

@mbohlool

mbohlool Jul 11, 2018

Member

I feel this should be a hack/verify-* thing. Is this being discussed? we are doing verify work in makefile which is odd.

This comment has been minimized.

@roycaihw

roycaihw Jul 11, 2018

Author Member

We integrated the API rule checking into OpenAPI spec generation (in kube-openapi) to provide an "API compiler / toolchain". In Kubernetes we add an extra layer to allow checking existing violation exceptions, but I think we still want new violations to fail in the "compiling" step. If we move the diff to hack/verify-openapi-spec.sh we will have a standalone hack/update-openapi-spec.sh script that passes on new violations (although our CI ensures verification scripts always get run, so violations will get caught eventually).

]),
go_deps = deps,
tools = ["//vendor/k8s.io/code-generator/cmd/openapi-gen"],

This comment has been minimized.

@mbohlool

mbohlool Jul 11, 2018

Member

we should also remove openapi-gen from code-generator repo after this PR got merged.

This comment has been minimized.

@roycaihw

roycaihw Jul 11, 2018

Author Member

Sounds good

@roycaihw

This comment has been minimized.

Copy link
Member Author

roycaihw commented Jul 11, 2018

/retest

@roycaihw roycaihw referenced this pull request Jul 12, 2018

Closed

Fix RBD Go field names #66097

@mbohlool

This comment has been minimized.

Copy link
Member

mbohlool commented Jul 13, 2018

/lgtm /approve

@roycaihw

This comment has been minimized.

Copy link
Member Author

roycaihw commented Jul 13, 2018

/assign @cblecker @thockin
for approval

@thockin

This comment has been minimized.

Copy link
Member

thockin commented Jul 13, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jul 13, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mbohlool, roycaihw, thockin

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

The pull request process is described here

Needs approval from an approver in each of these files:

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

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented Jul 14, 2018

Automatic merge from submit-queue (batch tested with PRs 64181, 65737). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit 614e3ad into kubernetes:master Jul 14, 2018

17 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation roycaihw authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Jul 16, 2018

I suspect this has broken the hack/update-generated-protobuf.sh script, I get the following error:

unknown shorthand flag: 'r' in -r
Usage of _output/bin/openapi-gen:
      --alsologtostderr                  log to standard error as well as files
      --build-tag string                 A Go build tag to use to identify files generated by this command. Should be unique. (default "ignore_autogenerated")
  -h, --go-header-file string            File containing boilerplate header text. The string YEAR will be replaced with the current 4-digit year. (default "/go/src/k8s.io/kubernetes/_output/dockerized/go/src/k8s.io/kubernetes/vendor/k8s.io/code-generator/hack/boilerplate.go.txt")
  -i, --input-dirs strings               Comma-separated list of import paths to get input types from.
      --log_backtrace_at traceLocation   when logging hits line file:N, emit a stack trace (default :0)
      --log_dir string                   If non-empty, write log files in this directory
      --logtostderr                      log to standard error instead of files (default true)
  -o, --output-base string               Output base; defaults to $GOPATH/src/ or ./ if $GOPATH is not set. (default "/go/src/k8s.io/kubernetes/_output/dockerized/go/src")
  -O, --output-file-base string          Base name (without .go suffix) for output files. (default "openapi_generated")
  -p, --output-package string            Base package path.
      --stderrthreshold severity         logs at or above this threshold go to stderr (default 2)
  -v, --v Level                          log level for V logs (default 0)
      --verify-only                      If true, only verify existing output, do not write anything.
      --vmodule moduleSpec               comma-separated list of pattern=N settings for file-filtered logging
unknown shorthand flag: 'r' in -r
@apelisse

This comment has been minimized.

Copy link
Member

apelisse commented Jul 16, 2018

Looks like I needed to run make clean, thanks

@roycaihw roycaihw deleted the roycaihw:api-linter branch Oct 1, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.