-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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 plugins based CLI #2827
Add plugins based CLI #2827
Conversation
bc4ec9f
to
dea45bd
Compare
dea45bd
to
64c4c78
Compare
This can finally use some reviews. In particular please go through the quickstart guide to verify the steps (skip the advanced sections), and look for typos/rephrasing etc. I am having some trouble with the sample-memcached example in the last step where the reconciler is failing to update the CR status: 2020-04-14T21:14:42.978-0700 ERROR controllers.Memcached Failed to update Memcached status {"memcached": "default/memcached-sample", "error": "memcacheds.cache.example.com \"memcached-sample\" not found"} Debugging this to see if it isn't just my Kind cluster but I could use a verification on the updated sample-memcached reconciler code. |
scorecard.NewCmd(), | ||
test.NewCmd(), | ||
version.NewCmd(), | ||
// TODO(hasbro17): add generate csv command after aligning it for kubebuilder layout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes here shows ok for me. IHMO:
- We could take advantage and create the testdata dir to scaffold a project to test it as it is done in the kubebuilder when we run the command
make generate
and the add the CI check as well to ensure that testdata is updated with the changes. See: the shell to gen the mock testdata and the shell that is used to check it in the CI here. Then, it would more less a copy and paste which would allow we to check the projects scaffolded with, help us to verify the PR and ensure its quality. We also can run its tests as well. - Also, I do not think that we can merge this PR in the master until plugin phase 1 be merged there as well.
c/c @estroz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could take advantage and create the testdata dir to scaffold a project to test it as it is done in the kubebuilder
IMO we shouldn't duplicate kubebuilder tests. Once we get to the point of making additional tweaks or changes to the Kubebuilder scaffolds or when we add our own plugins with custom layouts, it might make sense to add similar tests. Until then, I don't see how it adds any value, unless I'm misunderstanding something.
Also, I do not think that we can merge this PR in the master until plugin phase 1 be merged there as well.
Typically I would agree with this, but I think in this case, it might make sense to push forward on this since:
- We're not making the KB CLI the default yet
- We're just now introducing the KB dependency, so our existing legacy code is not affected by the use of a feature branch commit.
What do others think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HI @joelanford,
I got your point of view over merge based on the branch and then it is OK for me.
Regards the tests I really think that we should add them as soon as possible because as you said now we have nothing on top but we will have it. However, IHMO it is not a blocker to merge this PR at all. Would just make easier perform the review. I am checking it manually anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! Just a few of non-blocking nits.
in testing this, I noticed that the above get generated by default...expected? I guess it doesn't matter but users might ask about it. |
For this example replace the generated controller file controllers/memcached_controller.go with the example memcached_controller.go implementation. the link to memcached_controller.go appears to be broken in the quickstart doc. |
the quickstart guide doesn't specifically call out that you need to install kustomize, that might be good at the top of the quickstart unless I missed it somewhere else. same for the kubebuilder installation being a dependency. |
For this example we will run the operator in the default namespace which can be specified for all resources in config/default/kustomization.yaml: Adds namespace to all resources.namespace: default in testing, I noticed that it installs the operator into the namespace of 'memcached-operator-system' instead of default, so the quick start guide might need to be updated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please just check the comments:
- https://github.com/operator-framework/operator-sdk/pull/2827/files#r408935836 -
- https://github.com/operator-framework/operator-sdk/pull/2827/files#r408973355
- https://github.com/operator-framework/operator-sdk/pull/2827/files#r408983434
which are not a blocker for the merge since the goal is just to add the dep and getting started with.
Otherwise, it shows great. Good work 🥇
/lgtm
/approve
@jmccormick2001 Addressing your comments here but for follow ups it'll probably be better to add direct review comments on the PR files so I can respond to them more easily.
That's the default scaffold in Kubebuilder for the sample CR so it's expected.
That link is currently pointing to the file on the master branch which should work once this PR is merged.
Yep good point. I'll add that to the pre-reqs.
I'll make the following statement more actionable to get users to actually change the kustomization.yaml file so they can run in the default namespace. Probably with
# Adds namespace to all resources.
namespace: default |
return root | ||
} | ||
|
||
func getInitCmd() *cobra.Command { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Non-blocking note: the way init
is retrieved prevents global flags set by root, which contain --plugins
, from being retrieved. This is not blocking because right now only one plugin is available. Once kubernetes-sigs/kubebuilder#1465 merges, --plugins
will be local to init
, so we can bump kubebuilder commits to get --plugins
registration.
45c1be9
to
5299ec7
Compare
a84ad80
to
3b3bb24
Compare
Updated for review comments. Had to rebase for a conflict from master though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
after addressing nit
3b3bb24
to
ad5ed2b
Compare
New changes are detected. LGTM label has been removed. |
ad5ed2b
to
ac3e9a2
Compare
Description of the change:
Add a new CLI entrypoint based on Kubebuilder plugins.
Currently uses plugins from the feature/plugins-part-2-electric-boogaloo branch until kubernetes-sigs/kubebuilder#1469 is merged at which point we can update go.mod to use the master branch.
The
operator-sdk
CLI now has 2 entrypoints:cmd/operator-sdk/cli/legacy.go
now) which has been updated with a hiddenoperator-sdk init
command to allow scaffolding kubebuilder projects. Seeoperator-sdk init --project-version=2 -h
.cmd/operator-sdk/cli/cli.go
) which only shows once you're inside a Kubebuilder project with a validPROJECT
file.Once the integration is complete and we decide the CLI is ready for a release, we can change the order of the CLI entrypoints to make the Kubebulder aligned CLI the default, and deprecate the legacy CLI.
Also added
website/content/en/docs/kubebuilder/quickstart.md
as the new quickstart guide that is aligned with the new CLI and layout.This quickstart guide defers to upstream docs as much as possible since the kubebuilder docs are detailed enough for further reading.
This doc folder will remain hidden from the website, since it has no index, until the integration and corresponding docs are completed.
Some of the advanced sections like metrics and kube-state CR metrics don't apply directly anymore since the Kubebuilder default scaffolding doesn't use SDK helpers for metrics.
So I've marked those as TODOs and we'll need to figure out how to update/remove those docs.