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

Add restclientconfig helper fn for parsing timeout #37708

Merged
merged 1 commit into from
Dec 14, 2016
Merged

Add restclientconfig helper fn for parsing timeout #37708

merged 1 commit into from
Dec 14, 2016

Conversation

juanvallejo
Copy link
Contributor

Related downstream PR: openshift/origin#12062 (example of use-case for this patch)

Release note:

release-note-none

This patch adds a package pkg/client/unversioned/clientcmd/util and
defines a ParseTimeout helper function for parsing time from a
user-defined string. This allows code re-use in other packages that
require the creation of a new restclient (and therefore must set the
--global-timeout flag value manually).

@fabianofranz @kubernetes/cli-review

@k8s-ci-robot
Copy link
Contributor

Can a kubernetes member verify that this patch is reasonable to test? If so, please reply with "@k8s-bot 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 will still work. Regular contributors should join the org to skip this step.

If you have questions or suggestions related to this bot's behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Nov 30, 2016
@k8s-oncall
Copy link

This change is Reviewable

@juanvallejo juanvallejo mentioned this pull request Nov 30, 2016
Merged
@fabianofranz fabianofranz mentioned this pull request Nov 30, 2016
Closed
@k8s-github-robot k8s-github-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. release-note-label-needed labels Nov 30, 2016
@fabianofranz
Copy link
Contributor

@k8s-bot ok to test

fabianofranz
fabianofranz reviewed Dec 1, 2016
View reviewed changes
pkg/client/unversioned/clientcmd/util/helpers.go
limitations under the License.
*/

package util
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see the need for an util package. This can go directly in pkg/client/unversioned/clientcmd/helpers.go.

fabianofranz
fabianofranz reviewed Dec 1, 2016
View reviewed changes
pkg/client/unversioned/clientcmd/util/helpers.go
if requestTimeout, err := time.ParseDuration(duration); err == nil {
return requestTimeout, nil
}
return 0, fmt.Errorf("Invalid timeout value. Timeout must be a single integer, or an integer followed by a corresponding time unit (e.g. 1s | 2m | 3h)")
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this didn't change, but we could make it clear the unit when it's a single integer. Like "...must be a single integer in seconds, or an integer...".

@juanvallejo
Copy link
Contributor Author

@fabianofranz Thanks for the feedback, review comments addressed

@juanvallejo juanvallejo force-pushed the jvallejo/add-restclient-config-helper-for-parsing-global-timeout branch from 976f67c to e051bd9 Compare December 1, 2016 21:58
@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit 976f67cada78fcf80118984b8da83be193fc7acb. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@fabianofranz
Copy link
Contributor

Squash then LGTM.

@juanvallejo juanvallejo force-pushed the jvallejo/add-restclient-config-helper-for-parsing-global-timeout branch from e051bd9 to 0c975ce Compare December 2, 2016 14:48
@juanvallejo
Copy link
Contributor Author

@fabianofranz

Squash then LGTM.

Done!

@fabianofranz
Copy link
Contributor

@k8s-bot unit test this

@juanvallejo juanvallejo force-pushed the jvallejo/add-restclient-config-helper-for-parsing-global-timeout branch from 0c975ce to 22e1687 Compare December 2, 2016 15:24
@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit 0c975ceaaf0c9c7673c28b33139117243cdfc8eb. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins verification failed for commit 22e16875cfd7166d97f9bd0c8c0a5be6019d05a9. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@juanvallejo juanvallejo force-pushed the jvallejo/add-restclient-config-helper-for-parsing-global-timeout branch from 22e1687 to c92e14a Compare December 2, 2016 16:15
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE etcd3 e2e failed for commit c92e14a19ff481237a67bf1cbed08a235e181ca4. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce etcd3 e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit 22e16875cfd7166d97f9bd0c8c0a5be6019d05a9. Full PR test history.

The magic incantation to run this job again is @k8s-bot cvm gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@juanvallejo
Add restclientconfig helper fn for parsing timeout
885b7a6 
This patch adds a package `pkg/client/unversioned/clientcmd/util` and
defines a `ParseTimeout` helper function for parsing time from a
user-defined string. This allows code re-use in other packages that
require the creation of a new restclient (and therefore must set the
`--global-timeout` flag value manually).
@juanvallejo juanvallejo force-pushed the jvallejo/add-restclient-config-helper-for-parsing-global-timeout branch from c92e14a to 885b7a6 Compare December 2, 2016 18:25
@k8s-ci-robot
Copy link
Contributor

Jenkins kops AWS e2e failed for commit 885b7a6. Full PR test history.

The magic incantation to run this job again is @k8s-bot kops aws e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@juanvallejo
Copy link
Contributor Author

juanvallejo commented Dec 5, 2016 •
edited
Loading

@lavalamp or @fabianofranz would this patch be okay to tag?

@fabianofranz
Copy link
Contributor

@k8s-bot kops aws e2e test this

@k8s-github-robot k8s-github-robot added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Dec 10, 2016
@fabianofranz fabianofranz added 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. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Dec 14, 2016
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 37708, 34410)

@k8s-github-robot k8s-github-robot merged commit 4d467de into kubernetes:master Dec 14, 2016
@juanvallejo juanvallejo deleted the jvallejo/add-restclient-config-helper-for-parsing-global-timeout branch December 14, 2016 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fabianofranz fabianofranz fabianofranz left review comments

Assignees

@lavalamp lavalamp

Labels
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/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@juanvallejo @k8s-ci-robot @k8s-oncall @fabianofranz @k8s-github-robot @lavalamp