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

initial commit for klient helper package #15

Merged
merged 1 commit into from
Jun 14, 2021
Merged

initial commit for klient helper package #15

merged 1 commit into from
Jun 14, 2021

Conversation

ShwethaKumbla
Copy link
Member

What this PR does / why we need it

Initial implementation of rest.Config constructor functions which helps to get the client connection with the api-server.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 19, 2021
@k8s-ci-robot
Copy link
Contributor

Hi @ShwethaKumbla. Thanks for your PR.

I'm waiting for a kubernetes-sigs 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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 19, 2021
@ShwethaKumbla
Copy link
Member Author

/cc @vladimirvivien

@ShwethaKumbla ShwethaKumbla changed the title initial commit for klient helper package [WIP] initial commit for klient helper package May 20, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 20, 2021
@vladimirvivien
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels May 20, 2021
@vladimirvivien
Copy link
Contributor

@cpanato I think the CI job may be broken

make: *** No rule to make target 'test'.  Stop.

Any idea? Your help is appreciated.

@cpanato
Copy link
Member

cpanato commented May 20, 2021

@cpanato I think the CI job may be broken

make: *** No rule to make target 'test'.  Stop.

Any idea? Your help is appreciated.

@vladimirvivien we need to merge this: #14

@vladimirvivien
Copy link
Contributor

@cpanato Ha! Just approved #14.
Thanks.

@cpanato
Copy link
Member

cpanato commented May 20, 2021

@ShwethaKumbla can you rebase this PR and fix the conflict? thanks!

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 20, 2021
@ShwethaKumbla
Copy link
Member Author

@cpanato done rebase.

@ShwethaKumbla ShwethaKumbla force-pushed the helper-pkg branch 2 times, most recently from 016230a to 0671cfa Compare May 20, 2021 17:55
@ShwethaKumbla
Copy link
Member Author

/retest

@ShwethaKumbla
Copy link
Member Author

@cpanato @vladimirvivien
Do we have kube config file under /root/.kube/config in the test env where these jobs are running ?
What should be the ideal way to configure the kube config path in _test.go files ?

@cpanato
Copy link
Member

cpanato commented May 21, 2021 •
edited
Loading

@cpanato @vladimirvivien
Do we have kube config file under /root/.kube/config in the test env where these jobs are running ?
What should be the ideal way to configure the kube config path in _test.go files ?

no, it does not have, you might need to create it as part of the test.
but this is just the unit tests, when we need to test e2e (like enable the examples) we will need to run it on an environment that has a cluster deployed

@ShwethaKumbla ShwethaKumbla force-pushed the helper-pkg branch 4 times, most recently from bccc0c7 to 8552a19 Compare May 26, 2021 08:59
@ShwethaKumbla ShwethaKumbla changed the title [WIP] initial commit for klient helper package initial commit for klient helper package May 31, 2021
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 31, 2021
@ShwethaKumbla ShwethaKumbla mentioned this pull request Jun 1, 2021
Closed
vladimirvivien
vladimirvivien reviewed Jun 9, 2021
View reviewed changes
Copy link
Contributor

@vladimirvivien vladimirvivien left a comment

Choose a reason for hiding this comment

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

Overall looks good.
Letft some comments.

klient/conf/config.go Show resolved Hide resolved
klient/conf/config.go Show resolved Hide resolved
klient/conf/config.go Show resolved Hide resolved
klient/conf/config.go Outdated Show resolved Hide resolved
klient/conf/config.go Show resolved Hide resolved
klient/conf/main_test.go Show resolved Hide resolved
klient/conf/config_test.go Show resolved Hide resolved
andrewsykim
andrewsykim reviewed Jun 9, 2021
View reviewed changes
klient/conf/main_test.go Show resolved Hide resolved
klient/conf/config.go Outdated Show resolved Hide resolved
wilsonehusin
wilsonehusin reviewed Jun 10, 2021
View reviewed changes
klient/conf/config_test.go Outdated Show resolved Hide resolved
klient/conf/config_test.go Outdated Show resolved Hide resolved
klient/conf/main_test.go Outdated Show resolved Hide resolved
andrewsykim
andrewsykim reviewed Jun 11, 2021
View reviewed changes
klient/conf/config.go Outdated Show resolved Hide resolved
cpanato
cpanato reviewed Jun 14, 2021
View reviewed changes
Copy link
Member

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

lgtm, thanks for that

one request: can you squash the commits?

vladimirvivien
vladimirvivien reviewed Jun 14, 2021
View reviewed changes
Copy link
Contributor

@vladimirvivien vladimirvivien left a comment

Choose a reason for hiding this comment

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

I only have one comment. Once resolved this is good to go!

klient/conf/config.go Outdated Show resolved Hide resolved
@vladimirvivien
Copy link
Contributor

+1 on the commit squashing!

vladimirvivien
vladimirvivien approved these changes Jun 14, 2021
View reviewed changes
Copy link
Contributor

@vladimirvivien vladimirvivien left a comment

Choose a reason for hiding this comment

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

LGTM

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 14, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ShwethaKumbla, vladimirvivien

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 Jun 14, 2021
@k8s-ci-robot k8s-ci-robot merged commit 3736def into kubernetes-sigs:master Jun 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wilsonehusin wilsonehusin wilsonehusin left review comments

@andrewsykim andrewsykim andrewsykim left review comments

@cpanato cpanato cpanato left review comments

@vladimirvivien vladimirvivien vladimirvivien approved these changes

Assignees

@vladimirvivien vladimirvivien

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. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. sig/testing Categorizes an issue or PR as relevant to SIG Testing. 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.
6 participants
@ShwethaKumbla @k8s-ci-robot @vladimirvivien @cpanato @andrewsykim @wilsonehusin