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

Removed scheduler dependencies to testapi. #48405

Merged
merged 1 commit into from
Jul 12, 2017
Merged

Removed scheduler dependencies to testapi. #48405

merged 1 commit into from
Jul 12, 2017

Conversation

k82cn
Copy link
Member

@k82cn k82cn commented Jul 3, 2017

What this PR does / why we need it:
When refactor scheduler to use client-go, k8s.io/api, it's also need to remove the dependeny to testapi.

prefer to only include import/BUILD changes for #44188, so created separated PR for other enhancement removal.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): partially fixes #44188

Release note:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 3, 2017
@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jul 3, 2017
@k82cn
Copy link
Member Author

k82cn commented Jul 3, 2017

And also check other components, no dependency to testapi.

grep -rn testapi test/integration/scheduler/*
grep -rn testapi test/e2e/scheduling/*
grep -rn testapi plugin/pkg/scheduler/*

@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 3, 2017
k82cn
k82cn commented Jul 3, 2017
View reviewed changes
plugin/pkg/scheduler/util/testutil.go
)

func init() {
if apiMediaType := os.Getenv("KUBE_TEST_API_TYPE"); len(apiMediaType) > 0 {
Copy link
Member Author

Choose a reason for hiding this comment

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

no sure whether we need such logic for scheduler; in scheduler, we only need to check against v1.

@k82cn
Copy link
Member Author

k82cn commented Jul 3, 2017

/retest

@k82cn k82cn changed the title Removed dependencies to testapi. Removed scheduler dependencies to testapi. Jul 3, 2017
sakeven
sakeven reviewed Jul 3, 2017
View reviewed changes
plugin/pkg/scheduler/util/testutil.go Outdated
return fmt.Sprintf("/api/%s/%s", g.externalGroupVersion.Version, resource)
}
return fmt.Sprintf("/api/%s/%s/%s", g.externalGroupVersion.Version, resource, name)
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

if block ends with a return statement, so drop this else and outdent its block?

@k82cn
Copy link
Member Author

k82cn commented Jul 4, 2017

/retest

@k82cn
Copy link
Member Author

k82cn commented Jul 4, 2017

/retest

@k82cn
Copy link
Member Author

k82cn commented Jul 5, 2017

@bsalamat , any suggestion?

@k82cn
Copy link
Member Author

k82cn commented Jul 10, 2017

@kubernetes/sig-scheduling-pr-reviews

@k8s-ci-robot k8s-ci-robot added the sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. label Jul 10, 2017
@bsalamat
Copy link
Member

@k82cn So, the new testutil.go file is just a copy/paste of pieces of testapi.go. Generally speaking, we should avoid duplicating code as much as we can. What necessitates this duplication?

@k82cn
Copy link
Member Author

k82cn commented Jul 10, 2017

This's part of #44188 , we should only dependent on client-go, k8s.io/api or other 3rd part lib; and testapi should not be in dependent list, so I move its code here.

@bsalamat
Copy link
Member

In that case, LGTM.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 11, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, k82cn

Associated issue: 44188

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

@k82cn
Copy link
Member Author

k82cn commented Jul 11, 2017

/retest

@fejta
Copy link
Contributor

fejta commented Jul 11, 2017

/test pull-kubernetes-e2e-kops-aws

gyliu513
gyliu513 reviewed Jul 12, 2017
View reviewed changes
plugin/pkg/scheduler/util/testutil.go
@@ -0,0 +1,168 @@
/*
Copyright 2017 The Kubernetes Authors.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some comments for this file:

  1. If want to put the util here, then we should name it as test_utils.go to keep consistent.

  2. But seems putting this to the folder of testing is better, we can just name the file as testapi.go.

Copy link
Member Author

Choose a reason for hiding this comment

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

try avoid one dir one file :).

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 48405, 48742, 48748, 48571, 48482)

@k8s-github-robot k8s-github-robot merged commit eb196f8 into kubernetes:master Jul 12, 2017
@k82cn k82cn deleted the k8s_44188_1 branch July 12, 2017 22:57
@caesarxuchao
Copy link
Member

@k82cn do you know why the bot added the "releate-note" label while this PR doesn't have a release note?

@k82cn
Copy link
Member Author

k82cn commented Jul 15, 2017

nop; I used release-note-none in description, but seems release-note label is marked.

@gyliu513
Copy link
Contributor

My understanding for this is:

  1. If do not specify NONE in Release note as this PR, the commit summary will be treated as release note.
    image

  2. We should always specify NONE in Release note or setting /release-note-none for PRs do not want to have release note.

@k82cn
Copy link
Member Author

k82cn commented Jul 15, 2017

@caesarxuchao , it seems we did not support following note in description.

```release-note-none
```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gyliu513 gyliu513 gyliu513 left review comments

@sakeven sakeven sakeven left review comments

Assignees

@davidopp davidopp

@bsalamat bsalamat

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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 Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Updated scheduler to use client-go
9 participants
@k82cn @bsalamat @k8s-github-robot @fejta @caesarxuchao @gyliu513 @sakeven @davidopp @k8s-ci-robot