Skip to content
This repository was archived by the owner on May 25, 2023. It is now read-only.

Add type Argument with some common parse function #705

Merged
merged 1 commit into from
Apr 4, 2019
Merged

Add type Argument with some common parse function #705

merged 1 commit into from
Apr 4, 2019

Conversation

lmzqwer2
Copy link
Contributor

@lmzqwer2 lmzqwer2 commented Apr 4, 2019

What this PR does / why we need it:

  1. add special type for plugin arguments
  2. add common parse function GetInt

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 #704

Special notes for your reviewer:

Release note:

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
  • If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: [email protected]

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-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Apr 4, 2019
@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Apr 4, 2019
k82cn
k82cn reviewed Apr 4, 2019
View reviewed changes
pkg/scheduler/framework/arguments.go
value, err := strconv.Atoi(argv)
if err != nil {
glog.Warningf("Could not parse argument: %s for key %s, with err %v", argv, key, err)
return
Copy link
Contributor

@k82cn k82cn Apr 4, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

the return value is (int, error)

It's ok to me :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. :)

k82cn
k82cn reviewed Apr 4, 2019
View reviewed changes
pkg/scheduler/framework/arguments.go
@@ -0,0 +1,28 @@
package framework
Copy link
Contributor

Choose a reason for hiding this comment

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

add license there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copy license from other file.

And I think the license may be outdated now?

Copyright 2018 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.

Could use 2019 for new added files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

New added files (arguments.go arguments_test.go) is now in license of 2019.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Apr 4, 2019
@k82cn
Copy link
Contributor

k82cn commented Apr 4, 2019

/lgtm
/approvve

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Apr 4, 2019
@k82cn
Copy link
Contributor

k82cn commented Apr 4, 2019

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 4, 2019
@k8s-ci-robot k8s-ci-robot merged commit ef96fea into kubernetes-retired:master Apr 4, 2019
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
@k8s-ci-robot
Merge pull request kubernetes-retired#705 from lmzqwer2/plugin_arguments
a613889 
Add type Argument with some common parse function
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
@k8s-ci-robot
Merge pull request kubernetes-retired#705 from lmzqwer2/plugin_arguments
4b6cb58 
Add type Argument with some common parse function
kevin-wangzefeng pushed a commit to kevin-wangzefeng/scheduler that referenced this pull request Jun 28, 2019
@k8s-ci-robot
Merge pull request kubernetes-retired#705 from lmzqwer2/plugin_arguments
81fea08 
Add type Argument with some common parse function
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hex108 hex108 hex108 left review comments

@k82cn k82cn k82cn left review comments

@jinzhejz jinzhejz Awaiting requested review from jinzhejz

@tizhou86 tizhou86 Awaiting requested review from tizhou86

Assignees

@k82cn k82cn

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 Indicates that a PR is ready to be merged. 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.

Add type Argument with some common parse function
4 participants
@lmzqwer2 @k8s-ci-robot @k82cn @hex108