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.

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

✨ add kubebuilder/tooling and controller setup to use binaries from the project bin (go/v3-alpha) #1711

Conversation

camilamacedo86
Copy link
Member

@camilamacedo86 camilamacedo86 commented Oct 6, 2020

Description

  • add hack/tooling to setup the binaries in the bin project when the KUBEBUILDER_ASSETS is not setup
  • update the quick start to make clear that the current steps to get the binaries still required until the CLI version v2.3.1

Motivation

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 6, 2020
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2020
@camilamacedo86 camilamacedo86 changed the title ✨ applying changes made for v2 to setup envtest via target w… ✨ applying changes made for v2 to setup envtest via target Oct 6, 2020
docs/book/src/quick-start.md Outdated Show resolved Hide resolved
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Oct 6, 2020

Hi @DirectXMan12, @gabbifish, @pwittrock,

We agreed just push bug fixes to v2+. However, it is not a breaking change and would allow v2 plugin projects able to work with the kubebuilder binary if it wasn't installed from a tarball.

So, are you ok with we do this change in the v2+?

Copy link
Contributor

@gabbifish gabbifish left a comment

Choose a reason for hiding this comment

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

/approve

I ran make test as well and all your introduced changes look good! :)

Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

comment inline, plus as was pointed out in the meeting, we should verify the downloaded tarball.

pkg/plugin/v2/scaffolds/init.go Outdated Show resolved Hide resolved
@DirectXMan12
Copy link
Contributor

/hold

re: pointing at "master", we can bcckport the script to a stable version if need-be

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2020
@camilamacedo86 camilamacedo86 changed the title ✨ applying changes made for v2 to setup envtest via target WIP: ✨ applying changes made for v2 to setup envtest via target Oct 8, 2020
@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 Oct 8, 2020
@camilamacedo86 camilamacedo86 force-pushed the plugin-v2-test-target branch 2 times, most recently from b3e1063 to a846b51 Compare October 10, 2020 13:07
@k8s-ci-robot k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 10, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 10, 2020
@camilamacedo86 camilamacedo86 changed the title WIP: ✨ applying changes made for v2 to setup envtest via target ✨ applying changes made for v2 to setup envtest via target Oct 10, 2020
@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 Oct 10, 2020
@camilamacedo86 camilamacedo86 changed the title ✨ applying changes made for v2 to setup envtest via target ✨ add kubebuilder/tooling and controller setup to use binaries from the project bin Oct 10, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 13, 2020
@camilamacedo86
Copy link
Member Author

/test pull-kubebuilder-e2e-k8s-1-17-0

@DirectXMan12
Copy link
Contributor

I'd prefer a runnable Go program to a shell script, I think (there's just so many things that can go wrong in a shell script, with a go run invocation orgs can use a custom module proxy to ensure that they're getting what they expect, etc).

That being said, I think I put on one of these scripts that we should probably install to GOBIN, and then symlink the particular version to bin/whatever or something, so we still share versions when appropriate -- having 27 copies of the same binary in ~ is not ok.

If we want that to live in a separate module in controller-runtime, that's fine.

If we want to migrate the kubernetes release branch to controller-runtime, and re-target the cloud build scripts, that's also probably fine (I don't really care strongly either way).

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 19, 2020
@estroz
Copy link
Contributor

estroz commented Nov 19, 2020

I'd prefer a runnable Go program to a shell script

In general I agree.

That being said, I think I put on one of these scripts that we should probably install to GOBIN, and then symlink the particular version to bin/whatever or something, so we still share versions when appropriate -- having 27 copies of the same binary in ~ is not ok.

The library API could have "desired version" parameters that gets compared to the versions of GOBIN/PATH binaries, and will install any of those binaries locally if its version does not match the desired version.

If we want that to live in a separate module in controller-runtime, that's fine.

I'd prefer that, so there's a canonical way of installing envtest deps.

If we want to migrate the kubernetes release branch to controller-runtime, and re-target the cloud build scripts, that's also probably fine (I don't really care strongly either way).

Ultimately I'd like to do that as well, especially if c-r has the installation library you mentioned above.

@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 23, 2020
@camilamacedo86
Copy link
Member Author

camilamacedo86 commented Dec 3, 2020

Hi @estroz @DirectXMan12,

With the latest alpha controller-runtime release, we could simplify its solution. See that it was updated/chaged.

Regards the suggestions made by @estroz:

If we want to migrate the kubernetes release branch to controller-runtime, and re-target the cloud build scripts,
If we want that to live in a separate module in controller-runtime

It might make it harder to keep maintained but I am OK with. However, we have. current problems to solve and the suggested approach will take a while to be achieved. In this way, I think that we should do:

1 - Move forward with this solution to solve the current problems in KB
2 - Raise an issue against controller-runtime to starts to generate the binaries from there.
3 - Copy adapted if it is required the tooling done here to controller-runtime (after this PR get merged)
4 - After the next controller-runtime release with the suggestions made then, we can start to use it and change the KB code implementation.

WDYT?

@camilamacedo86 camilamacedo86 changed the title ✨ add kubebuilder/tooling and controller setup to use binaries from the project bin ✨ add kubebuilder/tooling and controller setup to use binaries from the project bin (go/v3-alpha) Dec 3, 2020
@camilamacedo86 camilamacedo86 force-pushed the plugin-v2-test-target branch 5 times, most recently from 506d992 to 678bf78 Compare December 3, 2020 02:39
@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Dec 3, 2020
@estroz
Copy link
Contributor

estroz commented Dec 15, 2020

Common install path and library suggestions: kubernetes-sigs/controller-runtime#1234

@k8s-ci-robot
Copy link
Contributor

@camilamacedo86: PR needs rebase.

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.

@camilamacedo86
Copy link
Member Author

Closing it a favour to kubernetes-sigs/controller-runtime#1234 and kubernetes-sigs/controller-runtime#1300 as discussed in the bug triage meeting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
5 participants