Skip to content
This repository has been archived by the owner on Jan 17, 2020. It is now read-only.

Support rest config to talk to the control plane #68

Closed
wants to merge 1 commit into from
Closed

Support rest config to talk to the control plane #68

wants to merge 1 commit into from

Conversation

mengqiy
Copy link
Contributor

@mengqiy mengqiy commented Sep 17, 2019 •
edited
Loading

  • ControlPlane can return a restConfig that targeting it. This is useful in environments that kubectl is not present.

fixes #67

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 17, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: mengqiy
To complete the pull request process, please assign hoegaarden
You can assign the PR to them by writing /assign @hoegaarden in a comment when ready.

The full list of commands accepted by this bot can be found 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

@mengqiy mengqiy force-pushed the restconfig branch 2 times, most recently from adb0f74 to 0862e8e Compare September 17, 2019 22:50
@mengqiy
Copy link
Contributor Author

mengqiy commented Sep 17, 2019

/cc @DirectXMan12

DirectXMan12
DirectXMan12 suggested changes Sep 17, 2019
View reviewed changes
integration/control_plane.go Outdated
c := &rest.Config{
Host: f.APIURL().String(),
ContentConfig: rest.ContentConfig{
NegotiatedSerializer: scheme.Codecs.WithoutConversion(),
Copy link

Choose a reason for hiding this comment

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

do we actually need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is required.
See: https://github.com/kubernetes/client-go/blob/ecd4f6fb8c21f85d3a0c0143fcbf83c662b6d106/rest/config.go#L284

@mengqiy mengqiy force-pushed the restconfig branch 2 times, most recently from e3464df to a25dd5b Compare September 17, 2019 23:52
@mengqiy
Copy link
Contributor Author

mengqiy commented Sep 18, 2019

/cc @hoegaarden

@hoegaarden
Copy link
Contributor

That looks great!

However, when we started out with this, one of the goals we had was to make this thing be vendor-able into k/k, so we could refactor some tests to use this module (instead of some bash scripts and other things). So this would mean that we cannot depend on client-go or anything from staging. See also:

So we might want to revisit:

Do we still need to be vendor-able into k/k?

  • If not:
    • Do we need to introduce branching to allow support for all supported kubernetes/client-go versions?
    • Do we just want to support master of client-go?

@DirectXMan12
Copy link

Do we still need to be vendor-able into k/k?

This is a hypothetical, right? I didn't see anything currently in k/k using this.

Do we just want to support master of client-go?

I think this is probably fine in the short term, especially considering the practical chance of non-compatibility is fairly low, depending on how things are structured (i.e. it's not using anything super-intricate) -- for instance, we could use a dynamic client w/ unstructured instead of the generated clients.

@hoegaarden
Copy link
Contributor

This is a hypothetical, right? I didn't see anything currently in k/k using this.

sigs.k8s.io/testing_frameworks is, at least as far as I know, NOT used ink/k for now. However enabling that was one of the goals when we started out. The only consumer of this package, to my knowledge, actually is kubebuilder/controller-runtime/...

How would you all fell about:

  • deprecating sigs.k8s.io/testing_frameworks
  • move & integrate the code into sigs.k8s.io/controller-runtime/pkg/envtest

It seems envtest is doing a lot of things to make testing_frameworks/integration work well with the controller-runtime setup. If my assumption is correct and controller-runtime is the only real user, we could as well just move the code there and treat envtest/controller-runtime as what it is: the main (only?) user/consumer of this lib.

cc: @apelisse @marun @DirectXMan12

A quick glance over https://cs.k8s.io/?q=testing_frameworks seems to suggest that this package is used in places where controller-runtime is used and it gets pulled in as a transitive dependency.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 26, 2019
@mengqiy mengqiy changed the title Support rest config and go modules Support rest config to talk to the control plane Sep 26, 2019
@k8s-ci-robot
Copy link
Contributor

@mengqiy: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-frameworks-test ca1ea39 link /test pull-frameworks-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@DirectXMan12
Copy link

I think we're fine with that from the controller-runtime side :-)

@DirectXMan12 DirectXMan12 mentioned this pull request Oct 16, 2019
Closed
@hoegaarden hoegaarden mentioned this pull request Nov 11, 2019
Closed
3 tasks
@fejta-bot
Copy link

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

@k8s-ci-robot k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 29, 2019
@mengqiy
Copy link
Contributor Author

mengqiy commented Jan 10, 2020

I will move this PR to controller-runtime.
Ref: kubernetes-sigs/controller-runtime#749

@mengqiy mengqiy closed this Jan 10, 2020
@mengqiy mengqiy deleted the restconfig branch January 10, 2020 23:32
@mengqiy mengqiy mentioned this pull request Jan 10, 2020
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DirectXMan12 DirectXMan12 DirectXMan12 requested changes

@apelisse apelisse Awaiting requested review from apelisse

@totherme totherme Awaiting requested review from totherme

@hoegaarden hoegaarden Awaiting requested review from hoegaarden

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. 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.

Support a pre-configured client-go client
5 participants
@mengqiy @k8s-ci-robot @hoegaarden @DirectXMan12 @fejta-bot