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.

Sign up for GitHub

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

Jump to bottom

Split APIVersion into APIGroup and APIVersion in audit events #50007

Merged
merged 2 commits into from
Sep 1, 2017
Merged

Split APIVersion into APIGroup and APIVersion in audit events #50007

merged 2 commits into from
Sep 1, 2017

Conversation

CaoShuFeng
Copy link
Contributor

@CaoShuFeng CaoShuFeng commented Aug 2, 2017 •
edited
Loading

audit.Event.ObjectRef.APIVersion currently holds both the the API group and
version, separated by a /. This change break these out into separate fields.

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 2, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @CaoShuFeng. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

@k8s-github-robot k8s-github-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 2, 2017
@CaoShuFeng
Copy link
Contributor Author

I haven't figure out how to make a pull request depends on another one!
This one contains tow commits from #49115 which is not expected.

@CaoShuFeng CaoShuFeng changed the title Split APIVersion into APIGroup and APIVersion in audit events [WIP]Split APIVersion into APIGroup and APIVersion in audit events Aug 2, 2017
@crassirostris
Copy link

I haven't figure out how to make a pull request depends on another one!
This one contains tow commits from #49115 which is not expected.

I don't think it's possible actually. I suggest just leaving the commit there until #49115 is merged and then rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 8, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Aug 18, 2017
@CaoShuFeng
Copy link
Contributor Author

@crassirostris @sttts @ericchiang
This is ready for review. Please take a look.
Thanks.

@CaoShuFeng CaoShuFeng changed the title [WIP]Split APIVersion into APIGroup and APIVersion in audit events Split APIVersion into APIGroup and APIVersion in audit events Aug 18, 2017
@crassirostris
Copy link

audit.Event.ObjectRef.APIVersion is splited into audit.Event.ObjectRef.APIGroup and audit.Event.ObjectRef.APIVersion

splitTed

crassirostris
crassirostris reviewed Aug 18, 2017
View reviewed changes
Copy link

@crassirostris crassirostris left a comment

Choose a reason for hiding this comment

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

LGTM

staging/src/k8s.io/apiserver/pkg/apis/audit/v1alpha1/conversion_test.go Outdated
func TestConversion(t *testing.T) {
scheme.Log(t)

testcases := map[string]struct {
Copy link

Choose a reason for hiding this comment

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

Nit: To be consistent with the rest of the tests, use structs and add desc field

And later, use t.Run(tc.desc, func () { ... }) to populate description to the logs instead of manually adding it to the format

@CaoShuFeng
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-etcd3
/test pull-kubernetes-unit

@CaoShuFeng
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-etcd3
/test pull-kubernetes-unit

@crassirostris crassirostris removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 20, 2017
@crassirostris
Copy link

@CaoShuFeng LGTM

One nit: WDYT about making the tests uniform, i.e. run them using t.Run everywhere?

@CaoShuFeng
Copy link
Contributor Author

One nit: WDYT about making the tests uniform, i.e. run them using t.Run everywhere?

I think this would take too much time and t.Run() is not used everywhere is this project workspace..

@crassirostris
Copy link

crassirostris commented Aug 21, 2017 •
edited
Loading

@CaoShuFeng

I think this would take too much time and t.Run() is not used everywhere is this project workspace..

As I've mention in another PR, if there's no kubernetes-wide policy about tests, do as you wish, that's just my feeling that writing t.Run once is better than appending description in each log line, at least for new/modified tests

Also, here are examples of such convention used in Kubernetes: https://github.com/kubernetes/kubernetes/blob/master/pkg/kubelet/kubelet_pods_test.go#L158 https://github.com/kubernetes/kubernetes/blob/master/pkg/api/v1/helper/helpers_test.go#L51 https://github.com/kubernetes/kubernetes/blob/master/pkg/auth/nodeidentifier/default_test.go#L58

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 27, 2017
@crassirostris
Copy link

@CaoShuFeng What's the status of this PR?

@CaoShuFeng
Copy link
Contributor Author

I will rebase this change.
(Maybe after #48836 gets merged.)

sttts
sttts reviewed Aug 30, 2017
View reviewed changes
staging/src/k8s.io/apiserver/pkg/apis/audit/v1beta1/types.go
// APIGroup is the name of the API group that contains the referred object.
// The empty string represents the core API group.
// +optional
APIGroup string `json:"apiGroup,omitempty" protobuf:"bytes,5,opt,name=apiGroup"`
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong protobuf index. We cannot reuse 5.

@sttts
Copy link
Contributor

sttts commented Aug 30, 2017

The protobuf index must be updated.

I don't think we need the release-note if we introduce a v1beta1 group. We should have one "big" release-note for the new group in the PR which switches it on.

@sttts sttts added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 30, 2017
@CaoShuFeng
Split APIVersion into APIGroup and APIVersion in audit events
c57eebf 
audit.Event.ObjectRef.APIVersion currently holds both the the API group and
version, separated by a /. This change break these out into separate fields.

This is part of:
kubernetes#48561
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2017
@CaoShuFeng
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@CaoShuFeng
Copy link
Contributor Author

/test pull-kubernetes-e2e-gce-etcd3

@CaoShuFeng
Copy link
Contributor Author

/test pull-kubernetes-e2e-kops-aws

@sttts
Copy link
Contributor

sttts commented Aug 31, 2017

/retest

@sttts
Copy link
Contributor

sttts commented Aug 31, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 31, 2017
@sttts
Copy link
Contributor

sttts commented Aug 31, 2017

/approve no-issue

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: CaoShuFeng, sttts

Associated issue requirement bypassed by: sttts

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2017
@sttts
Copy link
Contributor

sttts commented Aug 31, 2017

/retest

2 similar comments
@sttts
Copy link
Contributor

sttts commented Aug 31, 2017

/retest

@sttts
Copy link
Contributor

sttts commented Aug 31, 2017

/retest

@soltysh soltysh added this to the v1.8 milestone Aug 31, 2017
@soltysh
Copy link
Contributor

soltysh commented Aug 31, 2017

/retest

@fejta-bot
Copy link

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51632, 51055, 51676, 51560, 50007)

@k8s-github-robot k8s-github-robot merged commit 92db97d into kubernetes:master Sep 1, 2017
@CaoShuFeng CaoShuFeng mentioned this pull request Sep 2, 2017
Merged
@shyamjvs shyamjvs mentioned this pull request Sep 4, 2017
Closed
@CaoShuFeng CaoShuFeng deleted the apiversion_apigroup branch November 6, 2017 05:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crassirostris crassirostris crassirostris left review comments

@sttts sttts sttts left review comments

Assignees

@ncdc ncdc

@sttts sttts

@tallclair tallclair

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/audit cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.8
Development

Successfully merging this pull request may close these issues.
10 participants
@CaoShuFeng @k8s-ci-robot @crassirostris @sttts @k8s-github-robot @soltysh @fejta-bot @ncdc @ericchiang @tallclair