Skip to content

Conversation

hasbro17
Copy link
Contributor

[skip ci]

Description of the change:

  • Added a migration guide to migrate a Go operator project to use SDK v0.1.0 with the new controller-runtime changes.
  • Update the user-guide for adding 3rd party resources.
  • The v0.1.0-changes.md doc referenced in the migration guide will be added in a followup PR. But even without that the migration guide should be sufficient to understand the changes.

Motivation for the change:

  • Need a migration guide before making the v0.1.0 release.

/cc @shawn-hurley @joelanford @lilic @AlexNPavel @estroz

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.

Flow looks good. A few cosmetic changes are needed.


### Scaffold api for custom types

Create the api for your custom resource(CR) in the new project with `operator-sdk add api --api-version=<apiversion> --kind=<kind>`
Copy link
Member

Choose a reason for hiding this comment

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

Space before (CR)

You then need to tell the operators to use these functions to add the resources to its scheme. In operator-sdk you use [AddToSDKScheme][osdk_add_to_scheme] to add this.
Example of you main.go:
```go
By default the operator's manager will register all custom resource types defined in your project under `pkg/apis` with its scheme.
Copy link
Member

Choose a reason for hiding this comment

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

{m -> M}anager

To add a 3rd party resource to an operator, you must add it to the manager's scheme. By creating an `AddToScheme` method or reusing one you can easily add a resource to your scheme. An [example][deployments_register] shows that you define a function and then use the [runtime][runtime_package] package to create a `SchemeBuilder`.

#### Register with the manager's scheme
Call the `AddToScheme()` function for your 3rd party resource and pass it the manager's scheme via `mgr.GetScheme()`.
Copy link
Member

Choose a reason for hiding this comment

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

{m -> M}anager

However if there are any operator specific flags or settings defined in the old main file copy those over.
If you have any 3rd party resource types registered with the sdk's scheme, then register those with the manager's scheme in the new project. See how to [register 3rd party resources][register-3rd-party-resources].
Copy link
Member

Choose a reason for hiding this comment

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

Upper-case 'sdk'

@openshift-ci-robot openshift-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 22, 2018
estroz and others added 6 commits October 22, 2018 13:48
Co-Authored-By: hasbro17 <hasbro17@gmail.com>
Co-Authored-By: hasbro17 <hasbro17@gmail.com>
Co-Authored-By: hasbro17 <hasbro17@gmail.com>
Co-Authored-By: hasbro17 <hasbro17@gmail.com>
Co-Authored-By: hasbro17 <hasbro17@gmail.com>
Co-Authored-By: hasbro17 <hasbro17@gmail.com>
@hasbro17 hasbro17 force-pushed the add-migration-guide branch from 21622d7 to ce43fd5 Compare October 22, 2018 21:20
@hasbro17 hasbro17 force-pushed the add-migration-guide branch from ce43fd5 to ec57205 Compare October 22, 2018 21:20
@hasbro17
Copy link
Contributor Author

Fixed some typos and formatting issues.

[v0.1.0-changes-doc]: ./v0.1.0-changes.md
[v0.0.7-memcached-operator]: https://github.com/operator-framework/operator-sdk-samples/tree/aa15bd278eec0959595e0a0a7282a26055d7f9d6/memcached-operator
[v0.1.0-memcached-operator]: https://github.com/operator-framework/operator-sdk-samples/tree/master/memcached-operator
Copy link
Contributor

Choose a reason for hiding this comment

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

This should probably be locked to a specific commit so this doc stays consistent as we update the SDK over time

Copy link
Member

@lilic lilic left a comment

Choose a reason for hiding this comment

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

Initial feedback, but didn't get a chance to try it yet.

@@ -0,0 +1,268 @@
# Migration Guide from v0.0.x to v0.1.0

This document describes how to migrate an operator project built using Operator SDK `v0.0.x` to the project structure required by `v0.1.0`. For an overview of the major changes in the `v0.1.0` project layout and APIs see the [v0.1.0 changes doc][v0.1.0-changes-doc].
Copy link
Member

Choose a reason for hiding this comment

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

The ./v0.1.0-changes.md doesn't seem to exist so let's add this when that is added.

err = r.client.Get(context.TODO(), types.NamespacedName{Name: name, Namespace: namespace}, dep)
```
Lastly copy and intialize any other fields that you may have had in your `Handler` struct into the `Reconcile<Kind>` struct:
Copy link
Member

Choose a reason for hiding this comment

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

s/intialize/initialize


#### Multiple custom resources

If your operator is watching more than 1 CR type then you can do one of the following depending on you application:
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If your operator is watching more than 1 CR type then you can do one of the following depending on you application:
If your operator is watching more than 1 CR type then you can do one of the following depending on your application:

Copy link
Member

@shawn-hurley shawn-hurley left a comment

Choose a reason for hiding this comment

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

LGTM just some thoughts but 👍 overall!

@hasbro17 hasbro17 changed the title doc: add migration guide [WIP] doc: add migration guide Oct 23, 2018
@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 Oct 23, 2018
@hasbro17 hasbro17 changed the title [WIP] doc: add migration guide doc: add migration guide Oct 23, 2018
@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 Oct 23, 2018
memcached-operator old-memcached-operator

# Copy over .git from old project
$ cp -rf memcached-operator/.git old-memcached-operator/.git
Copy link
Member

Choose a reason for hiding this comment

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

This copies new to old, right? Don't you want this:

cp -rf old-memcached-operator/.git memcached-operator/.git

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it should be the other way around, good catch.

@hasbro17 hasbro17 force-pushed the add-migration-guide branch 2 times, most recently from 871239f to 7d48f65 Compare October 25, 2018 00:16
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

@hasbro17 hasbro17 force-pushed the add-migration-guide branch 2 times, most recently from 8995a11 to 2738d46 Compare October 25, 2018 00:24
@hasbro17 hasbro17 force-pushed the add-migration-guide branch from 2738d46 to 281b24d Compare October 25, 2018 00:32
@hasbro17 hasbro17 merged commit 6d870c9 into operator-framework:master Oct 25, 2018
@hasbro17 hasbro17 deleted the add-migration-guide branch October 25, 2018 17:12
@hasbro17
Copy link
Contributor Author

Going to follow this up with the v0.1.0 vs v0.0.x differences doc. That should be a bit smaller since we've gone over most of it in the migration guide.

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

Labels

size/L Denotes a PR that changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants