-
Notifications
You must be signed in to change notification settings - Fork 0
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
Remove unused code, comments and update errors to use fmt.Errorf #67
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.
@berkayoz did a linter spot all these? Thanks for the clean up
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.
Amazing job @berkayoz! What a relief, thanks a lot! 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.
couple of comments. Thanks for cleaning up.
@@ -51,6 +46,7 @@ func (r *MachineReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manag | |||
|
|||
// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=clusters;clusters/status,verbs=get;list;watch | |||
// +kubebuilder:rbac:groups=cluster.x-k8s.io,resources=machines;machines/status,verbs=get;list;watch;create;update;patch;delete | |||
|
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.
I remember that having or not having this space caused issues in the past.
Does this fixes the issue or introduces one?
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.
Not having a space causes the issue as the comments are treated as function docs/comments. Seems like we already have these roles in the contolplane controller so this wasn't caught. Additionally the machine controller is no-op here as it used to handle etcd removal logic, AFAIK we could also remove it totally.
// so that the cluster does not lock and cause the cluster to go down. In the case of k8s-dqlite, this is automatically handled by the | ||
// go-dqlite layer, and Canonical Kubernetes has logic to automatically keep a quorum of nodes in normal operation. | ||
// | ||
// Therefore, we currently disable this check for simplicity, but should remember that we need this precondition before proceeing. |
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.
Hm, this note sounds not like a waste of space tbh
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.
I've removed it since it referred to the commented out code later on, instead I've updated to say we've removed this check.
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, great stuff!
Removes unused etcd related commented out logic, conditions and variables
Updated errors to use
fmt.Errorf("foo: %w", err)
Drop
github.com/pkg/errors
since standard library is enough for error wrapping currently