Skip to content

Conversation

hasbro17
Copy link
Contributor

@hasbro17 hasbro17 commented Jun 8, 2020

Make the new Kubebuilder aligned CLI the default for Go operators.

There are 3 main changes/commits in this PR to make it easier to review:

  • Make the new Go docs visible and mark the old ones as legacy:
    • Old/Legacy docs moved website/content/en/docs/golang/*=> website/content/en/docs/golang/legacy/*
    • New docs moved website/content/en/docs/kubebuilder/*=> website/content/en/docs/golang/new/*
    • Links updated in other docs accordingly
    • Link titles and weights added to the new docs to make them visible
  • Generate new CLI docs:
    • Updated hack/generate/cli-doc/gen-cli-doc.go to generate both the legacy and new CLI reference docs
    • Generated new CLI reference docs at website/content/en/docs/new-cli/
    • Old CLI docs can't be moved to not break links in CHANGELOG notes but marked as legacy in its _index.md.
  • Deprecate operator-sdk new --type=go and make the new KB CLI the default entrypoint outside of legacy projects:
    • main.go defaults to the new KB CLI except when running in legacy Go, or Ansible/Helm projects.
    • operator-sdk new --type=go has been removed so legacy Go projects cannot be scaffolded anymore. --type=Ansible/Helm still exist.
    • All commands in legacy Go projects will print out a Deprecation Notice and point to the new CLI and layout.

TODO:

  • Rebase once missing reference docs are added to the new docs in Add and align reference docs for new KB docs #3209
  • Inspect the rendered website docs to make sure all docs are visible with the correct link weights
  • Update/Disable test scripts using operator-sdk new --type=Go

Follow up PRs:

  • Remove internal Go scaffolding code
  • Reshuffle docs to de-emphasize legacy Go docs by nesting them inside the new ones.
  • Add CHANGELOG fragment for v0.19.0 to summarize removal of new --type=Go and deprecation of old CLI.
  • Add note to the new quickstart guide on installing control-plane binaries (etcd, kube-apiserver etc) for envtest to run correctly.
@hasbro17 hasbro17 added the kubebuilder-integration Relates to rewriting the SDK in Kubebuilder plugin form label Jun 8, 2020
@hasbro17 hasbro17 removed the request for review from shawn-hurley June 8, 2020 07:54
Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

Can we also remove all of the now-unused Go scaffolds?

@hasbro17
Copy link
Contributor Author

hasbro17 commented Jun 8, 2020

Can we also remove all of the now-unused Go scaffolds?

@joelanford I intentionally left that out of this PR so reviewers could focus on verifying the more important docs, links and CLI changes.
I can add a TODO and do a follow up PR for removing the unused scaffold code once this gets merged.

Or since it's just straight up removing code I can tack it on at the very end if the PR size isn't too large.

@hasbro17
Copy link
Contributor Author

hasbro17 commented Jun 8, 2020

Something's off with the doc links. Going to fix that.
https://travis-ci.org/github/operator-framework/operator-sdk/jobs/695909825#L327

- /target/docs/contribution-guidelines/testing/travis-build/index.html  * internally linking to ../../../golang/quickstart#a-note-on-dependency-management, which does not exist (line 1113)  <a href="../../../golang/quickstart#a-note-on-dependency-management">note</a>  * linking to internal hash #a-note-on-dependency-management that does not exist (line 1113)  <a href="../../../golang/quickstart#a-note-on-dependency-management">note</a> - /target/docs/faq/index.html  * internally linking to /docs/golang/legacy/legacy/monitoring/prometheus/#garbage-collection, which does not exist (line 993) ...
@joelanford
Copy link
Member

Can we also remove all of the now-unused Go scaffolds?

@joelanford I intentionally left that out of this PR so reviewers could focus on verifying the more important docs, links and CLI changes.
I can add a TODO and do a follow up PR for removing the unused scaffold code once this gets merged.

Or since it's just straight up removing code I can tack it on at the very end if the PR size isn't too large.

Either way seems fine to me. Just wanted to make sure that is covered.

@camilamacedo86
Copy link
Contributor

Hi @hasbro17,

It s not passing in the tests. Shows that it missing update the links reference. Maybe can be easy to split the PR one for the docs and other for the code source.

Regards the code see:

cmd/operator-sdk/new/cmd.go:385:6: func `getDeps` is unused (unused) cmd/operator-sdk/new/cmd.go:414:6: func `validateProject` is unused (unused) 

Regards the docs, also see that it shows not accurate. It has now 2 installations and is missing the Golang menu. See:

Screenshot 2020-06-09 at 12 20 09

Copy link
Contributor

@jmccormick2001 jmccormick2001 left a comment

Choose a reason for hiding this comment

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

Create a validating or mutating Admission Webhook

I get a 404 on that link in the docs.

@jmccormick2001
Copy link
Contributor

Also see the advanced topics doc for more use cases and under the hood details.

I get a 404 on that link in the docs when served locally.

@jmccormick2001
Copy link
Contributor

Filter watch events using predicates

I get a 404 on that link in the docs when served locally.

@jmccormick2001
Copy link
Contributor

completely unrelated to this PR, but I found it interesting that no labels were added to the role/sa/binding resources whereby you could remove them in an explicit way using a label selector:

jeffmc@breve:/projects/memcached-operator$ kubectl get role --show-labels
NAME CREATED AT LABELS
memcached-operator-leader-election-role 2020-06-09T14:40:14Z
jeffmc@breve:
/projects/memcached-operator$ kubectl get role,rolebinding,sa --show-labels
NAME CREATED AT LABELS
role.rbac.authorization.k8s.io/memcached-operator-leader-election-role 2020-06-09T14:40:14Z

NAME ROLE AGE LABELS
rolebinding.rbac.authorization.k8s.io/memcached-operator-leader-election-rolebinding Role/memcached-operator-leader-election-role 26s

NAME SECRETS AGE LABELS
serviceaccount/default 1 27d

so, the line in the docs under cleanup:

$ kubectl delete role,rolebinding --all

could be sort of dangerous if you have other stuff running in the default namespace. Would be great if a unique label were applied, maybe the operator name or similar.

@hasbro17 hasbro17 changed the title Switch over to new CLI and deprecate operator-sdk new --type=go WIP: Switch over to new CLI and deprecate operator-sdk new --type=go Jun 9, 2020
@openshift-ci-robot openshift-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 Jun 9, 2020
@hasbro17
Copy link
Contributor Author

hasbro17 commented Jun 9, 2020

The broken links are due to a missing index file. Will push that fix along with other changes in a bit.

Maybe can be easy to split the PR one for the docs and other for the code source.

@camilamacedo86 I'd rather not split this up if possible because the CLI changes and the doc changes depend on each other.
Otherwise we could end up with deprecated docs on the master and the CLI won't reflect those changes.

@hasbro17 hasbro17 force-pushed the default-to-new-CLI-and-docs branch 4 times, most recently from 756034f to eb17e7b Compare June 10, 2020 01:56
@hasbro17 hasbro17 changed the title Switch over to new CLI and deprecate operator-sdk new --type=go WIP: Switch over to new CLI and deprecate operator-sdk new --type=go Jun 15, 2020
@openshift-ci-robot openshift-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 Jun 15, 2020
@hasbro17
Copy link
Contributor Author

So I was going to disable the old e2e test for Go operators completely now that we're using the new CLI (which already has an e2e test).

But I think we probably could keep it around even though the old CLI for Go projects is deprecated, it's not unsupported in existing projects.
So most likely if I just update operator-sdk new in the go scaffolding scripts to use operator-sdk v0.18.1 the rest of the old CLI can still be tested in an old project layout.

log.Print("Creating new operator project")
cmdOut, err := exec.Command("operator-sdk",
"new",
operatorName,
"--repo", testRepo,
"--skip-validation").CombinedOutput()
if err != nil {
log.Fatalf("Error: %v\nCommand Output: %s\n", err, string(cmdOut))
}

@hasbro17 hasbro17 changed the title WIP: Switch over to new CLI and deprecate operator-sdk new --type=go Switch over to new CLI and deprecate operator-sdk new --type=go Jun 15, 2020
@openshift-ci-robot openshift-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 Jun 15, 2020
@hasbro17
Copy link
Contributor Author

The docs and CLI part of this PR should be ready for reviews (based on the layout in #3190 (comment)).

I'll fix the old Go e2e test in the meanwhile.

Copy link
Member

@joelanford joelanford left a comment

Choose a reason for hiding this comment

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

/lgtm

Great job!

Comment on lines +14 to +16
TODO: Once versioned docs are supported on the website the CHANGELOG notes
that link to the legacy CLI should be updated before being removed on the master
branch.
Copy link
Member

Choose a reason for hiding this comment

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

@asmacdo FYI

weight: 2
---

**Note:** This guide is for the legacy CLI and project layout. See the [new docs][new_docs] for the [Kubebuilder aligned CLI][new_CLI] and project layout.
Copy link
Member

Choose a reason for hiding this comment

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

Given that this quickstart assumes operator-sdk new --type=go exists, should we remove it?

This is non-blocking and could be done in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah forgot about that.
There's some stuff in the FAQ talking about metrics as well.
I'll add a TODO to follow this up.

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jun 16, 2020
Copy link
Member

@estroz estroz left a comment

Choose a reason for hiding this comment

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

One nit otherwise

/lgtm

new.NewCmd(),

alpha.NewCmd(),
build.NewCmd(),
Copy link
Contributor

@camilamacedo86 camilamacedo86 Jun 16, 2020

Choose a reason for hiding this comment

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

The operator-sdk create api is not showing when we use the help. Not a blocker too. We can do it in a follow up too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a plugin issue right @estroz
You have to be inside an initialized project for that help to show up for the correct plugin version I believe.

$ operator-sdk create api -h Scaffold a Kubernetes API. For project-specific information, run this command in the root directory of a project. Note: unable to find configuration file, project must be initialized Usage:  operator-sdk create api [flags] Flags:  -h, --help help for api

And inside a project:

$ operator-sdk create api -h Scaffold a Kubernetes API by creating a Resource definition and / or a Controller. create resource will prompt the user for if it should scaffold the Resource and / or Controller. To only scaffold a Controller for an existing Resource, select "n" for Resource. To only define the schema for a Resource without writing a Controller, select "n" for Controller. After the scaffold is written, api will run make on the project. Usage:  operator-sdk create api [flags] Examples:  # Create a frigates API with Group: ship, Version: v1beta1 and Kind: Frigate  operator-sdk create api --group ship --version v1beta1 --kind Frigate ...
Copy link
Member

Choose a reason for hiding this comment

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

You need to set --project-version to output help for the default plugin. However I do see create listed in the available subcommands when I run operator-sdk -h regardless of where the binary is.

Copy link
Contributor

@camilamacedo86 camilamacedo86 Jun 17, 2020

Choose a reason for hiding this comment

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

I saw: create Scaffold a Kubernetes API or webhook 👍

Copy link
Contributor

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

terrific work @hasbro17
I added 2 comments which IMO are not blocker and we can check in a follow-up.

@hasbro17
Copy link
Contributor Author

Merging this. Will address the TODOs(in the description) for the CHANGELOG, docs layout reshuffle and Go scaffold removal in follow up PRs.

@hasbro17 hasbro17 merged commit e128b9e into operator-framework:master Jun 16, 2020
@hasbro17 hasbro17 deleted the default-to-new-CLI-and-docs branch June 17, 2020 00:51
@balasgit
Copy link

Any inputs "how to use the new CLI" for the operator based on GO ? any samples or docs ? thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kubebuilder-integration Relates to rewriting the SDK in Kubebuilder plugin form lgtm Indicates that a PR is ready to be merged.

7 participants