tokers commented on a change in pull request #386:
URL:
https://github.com/apache/apisix-ingress-controller/pull/386#discussion_r615490868
##########
File path: pkg/ingress/apisix_route.go
##########
@@ -16,23 +16,33 @@ package ingress
import (
"context"
+ "fmt"
"time"
"go.uber.org/zap"
+ v1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
+ "k8s.io/apimachinery/pkg/api/meta"
+ metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+ "k8s.io/client-go/kubernetes/scheme"
"k8s.io/client-go/tools/cache"
+ "k8s.io/client-go/tools/record"
"k8s.io/client-go/util/workqueue"
"github.com/apache/apisix-ingress-controller/pkg/kube"
+
"github.com/apache/apisix-ingress-controller/pkg/kube/apisix/client/clientset/versioned/typed/config/v2alpha1"
"github.com/apache/apisix-ingress-controller/pkg/log"
"github.com/apache/apisix-ingress-controller/pkg/types"
apisixv1
"github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
)
+const RouteController = "RouteController"
Review comment:
Since this constant is not exported, just using a small case.
```suggestion
const (
_routeController = "ApisixRouteController"
)
```
##########
File path: pkg/ingress/controller.go
##########
@@ -44,6 +44,17 @@ import (
apisixv1
"github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
)
+const (
+ // SuccessSynced is used when a resource is synced successfully
+ SuccessSynced = "SyncSuccessfully"
Review comment:
We don't have to export all these constants.
##########
File path: pkg/ingress/apisix_route.go
##########
@@ -198,14 +210,50 @@ func (c *apisixRouteController) sync(ctx context.Context,
ev *types.Event) error
}
func (c *apisixRouteController) handleSyncErr(obj interface{}, err error) {
+ event := obj.(*types.Event)
+ route := event.Object.(kube.ApisixRouteEvent).OldObject
+ // conditions
+ condition := metav1.Condition{
+ Type: "APISIXSynced",
Review comment:
Not a good name. It's should be a noun like `"ResourcesReady"`.
##########
File path: pkg/ingress/apisix_route.go
##########
@@ -198,14 +210,50 @@ func (c *apisixRouteController) sync(ctx context.Context,
ev *types.Event) error
}
func (c *apisixRouteController) handleSyncErr(obj interface{}, err error) {
+ event := obj.(*types.Event)
+ route := event.Object.(kube.ApisixRouteEvent).OldObject
+ // conditions
+ condition := metav1.Condition{
+ Type: "APISIXSynced",
+ }
if err == nil {
+ message := fmt.Sprintf(MessageResourceSynced, RouteController)
+ if route.GroupVersion() == kube.ApisixRouteV1 {
+ c.recorder.Event(route.V1(), v1.EventTypeNormal,
SuccessSynced, message)
+ } else if route.GroupVersion() == kube.ApisixRouteV2alpha1 {
+ c.recorder.Event(route.V2alpha1(), v1.EventTypeNormal,
SuccessSynced, message)
+ // build condition
+ condition.Reason = "SyncSuccessfully"
+ condition.Status = "True"
+ condition.Message = "Sync Successfully"
+ // set to status
+ routev2 := route.V2alpha1()
+ meta.SetStatusCondition(routev2.Status.Conditions,
condition)
+
v2alpha1.New(kube.GetApisixClient().ApisixV2alpha1().RESTClient()).ApisixRoutes(routev2.Namespace).
+ Update(context.TODO(), routev2, nil)
+ }
c.workqueue.Forget(obj)
return
}
log.Warnw("sync ApisixRoute failed, will retry",
zap.Any("object", obj),
zap.Error(err),
)
+ message := fmt.Sprintf(MessageResourceFailed, RouteController,
err.Error())
+ if route.GroupVersion() == kube.ApisixRouteV1 {
+ c.recorder.Event(route.V1(), v1.EventTypeWarning, FailedSynced,
message)
+ } else if route.GroupVersion() == kube.ApisixRouteV2alpha1 {
+ c.recorder.Event(route.V2alpha1(), v1.EventTypeWarning,
FailedSynced, message)
+ // build condition
+ condition.Reason = "SyncFailed"
+ condition.Status = "False"
Review comment:
Use `metav1.ConditionFalse`.
##########
File path: pkg/ingress/controller.go
##########
@@ -44,6 +44,17 @@ import (
apisixv1
"github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
)
+const (
+ // SuccessSynced is used when a resource is synced successfully
+ SuccessSynced = "SyncSuccessfully"
+ // MessageResourceSynced is used to specify controller
+ MessageResourceSynced = "%s synced successfully"
+ // FailedSynced is used when a resource synced failed
+ FailedSynced = "SyncFailed"
Review comment:
`SuccessSynced` and `FailedSynced` terms are not so good. Synced means
resources were pushed to APISIX, why still using another adjective to decorate
it?
```
_resourcesSynced = "ResourcesSynced"
_resourcesSyncAborted = "ResourcesSyncAborted"
```
##########
File path: pkg/ingress/controller.go
##########
@@ -44,6 +44,17 @@ import (
apisixv1
"github.com/apache/apisix-ingress-controller/pkg/types/apisix/v1"
)
+const (
+ // SuccessSynced is used when a resource is synced successfully
+ SuccessSynced = "SyncSuccessfully"
+ // MessageResourceSynced is used to specify controller
+ MessageResourceSynced = "%s synced successfully"
+ // FailedSynced is used when a resource synced failed
+ FailedSynced = "SyncFailed"
Review comment:
PS: we obey the uber go style guide.
##########
File path: pkg/ingress/apisix_route.go
##########
@@ -198,14 +210,50 @@ func (c *apisixRouteController) sync(ctx context.Context,
ev *types.Event) error
}
func (c *apisixRouteController) handleSyncErr(obj interface{}, err error) {
+ event := obj.(*types.Event)
+ route := event.Object.(kube.ApisixRouteEvent).OldObject
+ // conditions
+ condition := metav1.Condition{
+ Type: "APISIXSynced",
+ }
if err == nil {
+ message := fmt.Sprintf(MessageResourceSynced, RouteController)
+ if route.GroupVersion() == kube.ApisixRouteV1 {
+ c.recorder.Event(route.V1(), v1.EventTypeNormal,
SuccessSynced, message)
+ } else if route.GroupVersion() == kube.ApisixRouteV2alpha1 {
+ c.recorder.Event(route.V2alpha1(), v1.EventTypeNormal,
SuccessSynced, message)
+ // build condition
+ condition.Reason = "SyncSuccessfully"
+ condition.Status = "True"
Review comment:
Use `metav1.ConditionTrue`.
##########
File path: pkg/ingress/apisix_route.go
##########
@@ -198,14 +210,50 @@ func (c *apisixRouteController) sync(ctx context.Context,
ev *types.Event) error
}
func (c *apisixRouteController) handleSyncErr(obj interface{}, err error) {
+ event := obj.(*types.Event)
+ route := event.Object.(kube.ApisixRouteEvent).OldObject
+ // conditions
+ condition := metav1.Condition{
+ Type: "APISIXSynced",
+ }
if err == nil {
+ message := fmt.Sprintf(MessageResourceSynced, RouteController)
+ if route.GroupVersion() == kube.ApisixRouteV1 {
+ c.recorder.Event(route.V1(), v1.EventTypeNormal,
SuccessSynced, message)
+ } else if route.GroupVersion() == kube.ApisixRouteV2alpha1 {
+ c.recorder.Event(route.V2alpha1(), v1.EventTypeNormal,
SuccessSynced, message)
+ // build condition
+ condition.Reason = "SyncSuccessfully"
Review comment:
We already have constants? Why not using them?
##########
File path: pkg/ingress/apisix_tls.go
##########
@@ -117,6 +123,8 @@ func (c *apisixTlsController) sync(ctx context.Context, ev
*types.Event) error {
zap.Error(err),
zap.Any("ApisixTls", tls),
)
+ message := fmt.Sprintf(MessageResourceFailed, TlsController,
err.Error())
+ c.recorder.Event(tls, corev1.EventTypeWarning, FailedSynced,
message)
Review comment:
Why not processing them in `c.handleSyncErr`.
##########
File path: pkg/ingress/apisix_upstream.go
##########
@@ -122,6 +128,8 @@ func (c *apisixUpstreamController) sync(ctx
context.Context, ev *types.Event) er
svc, err := c.controller.svcLister.Services(namespace).Get(name)
if err != nil {
log.Errorf("failed to get service %s: %s", key, err)
+ message := fmt.Sprintf(MessageResourceFailed,
UpstreamController, err.Error())
Review comment:
Why not handling them in `c.handleSyncErr`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
