-
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
Update Advanced Topics Doc #2999
Update Advanced Topics Doc #2999
Conversation
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.
Some formatting stuff, nothing major.
Co-authored-by: Camila Macedo <cmacedo@redhat.com>
Co-authored-by: Camila Macedo <cmacedo@redhat.com>
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.
A few suggestions
@@ -60,14 +60,18 @@ import ( | |||
routev1 "github.com/openshift/api/route/v1" | |||
) | |||
|
|||
func main() { | |||
func init() { | |||
... | |||
|
|||
// Adding the routev1 | |||
if err := routev1.AddToScheme(mgr.GetScheme()); err != nil { |
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.
Remove this now that below lines were added.
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
@theishshah fix the suggestion made by @estroz then you should be good.
Co-authored-by: Eric Stroczynski <estroczy@redhat.com>
Co-authored-by: Eric Stroczynski <estroczy@redhat.com>
Co-authored-by: Eric Stroczynski <estroczy@redhat.com>
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
@@ -60,14 +61,15 @@ import ( | |||
routev1 "github.com/openshift/api/route/v1" |
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.
@theishshah This doc is still referencing the old SDK layout filepath. Can't comment there but a few lines above it says:
Call the `AddToScheme()` function for your 3rd party resource and pass it the Manager's scheme via `mgr.GetScheme()`
in `cmd/manager/main.go`.
Where cmd/manager/main.go
is now main.go
in kubebuilder's layout.
Can you please proof read the doc again for other such references.
@theishshah Can you also remove the TODO's from this doc as you address them in this PR.
|
|
||
> **// TODO:** Remove this section since we're no longer scaffolding main.go to use the SDK's `GenerateAndServeCRMetrics()` util in `pkg/kube-metrics`. | ||
|
||
### Handle Cleanup on Deletion | ||
|
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.
@theishshah There's still a TODO here in the finalizer section for updating the finalizer section of this doc with the new var names and signatures of the kubebuilder controller.
// TODO: Update finalizer reconcile code for kubebuilder's default reconciler imports and variable names
@theishshah progress on this? I'd like to get this in before defaulting to the new project 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.
Just fix code intention across the doc.
Otherwise, it shows ok for me.
|
||
The operator's Manager supports the core Kubernetes resource types as found in the client-go [scheme][scheme_package] package and will also register the schemes of all custom resource types defined in your project. | ||
|
||
```Go | ||
import ( | ||
"github.com/example-inc/memcached-operator/pkg/apis" | ||
cachev1alpha1 "github.com/example-inc/memcached-operator/api/v1alpha1 |
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.
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 finalizer code is missing.
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.
@theishshah Still some more changes.
Plus we still have a TODO in the leader election section.
// TODO: Update this section to remove leader-for-life option? Since it's no longer the default.
Just remove that line. Don't think we need to update the leader election content itself.
|
||
utilruntime.Must(clientgoscheme.AddToScheme(mgr.GetScheme())) | ||
|
||
utilruntime.Must(routev1.AddToScheme(mgr.GetScheme())) | ||
// +kubebuilder:scaffold:scheme |
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.
@theishshah Can you please fix this indentation issue and double check this. It still hasn't been fixed from the past 2 reviews.
} | ||
|
||
// Add finalizer for this CR | ||
if !contains(memcached.GetFinalizers(), memcachedFinalizer) { | ||
if err := r.addFinalizer(reqLogger, memcached); err != nil { | ||
return reconcile.Result{}, err | ||
return ctrl.Result{}, err | ||
} | ||
} | ||
|
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 still have a return reconcile.Result{}, nil
on Line 194 below this.
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
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 @theishshah,
It still missing fix the layout of the code snippets added. See: https://github.com/operator-framework/operator-sdk/blob/7013dade5f8227e80aaa7a8b57c9c7817434ac6f/website/content/en/docs/kubebuilder/advanced-topics.md. All should have the same indentation which is used in the other docs as well.
Otherwise all fine.
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.
Great work 👍
/lgtm
/approve
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.
@theishshah There are still parts of the reconciler code that are from the old scaffold.
But once you update those, then LGTM.
|
||
... | ||
|
||
return ctrl.Result{}, nil | ||
} | ||
|
||
func (r *ReconcileMemcached) finalizeMemcached(reqLogger logr.Logger, m *cachev1alpha1.Memcached) error { |
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.
func (r *ReconcileMemcached) finalizeMemcached(reqLogger logr.Logger, m *cachev1alpha1.Memcached) error { | |
func (r * MemcachedReconciler) finalizeMemcached(reqLogger logr.Logger, m *cachev1alpha1.Memcached) error { |
// of finalizers include performing backups and deleting | ||
// resources that are not owned by this CR, like a PVC. | ||
reqLogger.Info("Successfully finalized memcached") | ||
return nil | ||
} | ||
|
||
func (r *ReconcileMemcached) addFinalizer(reqLogger logr.Logger, m *cachev1alpha1.Memcached) error { |
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.
func (r *ReconcileMemcached) addFinalizer(reqLogger logr.Logger, m *cachev1alpha1.Memcached) error { | |
func (r * MemcachedReconciler) addFinalizer(reqLogger logr.Logger, m *cachev1alpha1.Memcached) error { |
controllerutil.AddFinalizer(m, memcachedFinalizer) | ||
|
||
// Update CR | ||
err := r.client.Update(context.TODO(), m) |
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.
err := r.client.Update(context.TODO(), m) | |
err := r.Update(context.TODO(), m) |
New changes are detected. LGTM label has been removed. |
Description of the change:
Update info in advanced topics doc.
Motivation for the change: