fix: NO_CHANGES state on empty change set

Signed-off-by: Michal Fiedorowicz <mfiedorowicz@netboxlabs.com>
netboxlabs · Jan 16, 2025 · 6a39423 · 6a39423
1 parent d6786ec
commit 6a39423
Show file tree

Hide file tree

Showing 2 changed files with 42 additions and 10 deletions.
diff --git a/diode-server/reconciler/ingestion_processor_internal_test.go b/diode-server/reconciler/ingestion_processor_internal_test.go
@@ -434,6 +434,41 @@ func TestIngestionProcessor_GenerateAndApplyChangeSet(t *testing.T) {
 			expectedStatus:      reconcilerpb.State_OPEN,
 			expectedError:       false,
 		},
+		{
+			name: "generate change set without changes",
+			ingestionLog: &reconcilerpb.IngestionLog{
+				Id:                 uuid.NewString(),
+				RequestId:          "cfa0f129-125c-440d-9e41-e87583cd7d89",
+				ProducerAppName:    "test-app",
+				ProducerAppVersion: "0.1.0",
+				SdkName:            "diode-sdk-go",
+				SdkVersion:         "0.2.0",
+				ObjectType:         "dcim.site",
+				Entity: &diodepb.Entity{
+					Entity: &diodepb.Entity_Site{
+						Site: &diodepb.Site{
+							Name: "Site A",
+						},
+					},
+				},
+				IngestionTs: time.Now().UnixNano(),
+				State:       reconcilerpb.State_QUEUED,
+			},
+			mockRetrieveObjectStateResponse: &netboxdiodeplugin.ObjectState{
+				ObjectType: "dcim.site",
+				ObjectID:   1,
+				Object: &netbox.DcimSiteDataWrapper{
+					Site: &netbox.DcimSite{
+						ID:   1,
+						Name: "Site A",
+						Slug: "site-a",
+					},
+				},
+			},
+			autoApplyChangesets: false,
+			expectedStatus:      reconcilerpb.State_NO_CHANGES,
+			expectedError:       false,
+		},
 	}
 
 	for _, tt := range tests {
@@ -457,12 +492,12 @@ func TestIngestionProcessor_GenerateAndApplyChangeSet(t *testing.T) {
 
 			ingestionLogID := int32(1)
 
-			mockRepository.On("UpdateIngestionLogStateWithError", ctx, ingestionLogID, reconcilerpb.State_OPEN, mock.Anything).Return(nil)
 			mockNbClient.On("RetrieveObjectState", ctx, mock.Anything).Return(tt.mockRetrieveObjectStateResponse, nil)
 			if tt.autoApplyChangesets {
-				mockRepository.On("UpdateIngestionLogStateWithError", ctx, ingestionLogID, tt.expectedStatus, mock.Anything).Return(nil)
+				mockRepository.On("UpdateIngestionLogStateWithError", ctx, ingestionLogID, reconcilerpb.State_OPEN, mock.Anything).Return(nil)
 				mockNbClient.On("ApplyChangeSet", ctx, mock.Anything).Return(tt.mockApplyChangeSetResponse, nil)
 			}
+			mockRepository.On("UpdateIngestionLogStateWithError", ctx, ingestionLogID, tt.expectedStatus, mock.Anything).Return(nil)
 			mockRepository.On("CreateChangeSet", ctx, mock.Anything, ingestionLogID).Return(int32Ptr(1), nil)
 
 			bufCapacity := 1

diff --git a/diode-server/reconciler/ops.go b/diode-server/reconciler/ops.go
@@ -67,17 +67,14 @@ func (o *Ops) GenerateChangeSet(ctx context.Context, ingestionLogID int32, inges
 		return nil, nil, err
 	}
 
+	state := reconcilerpb.State_OPEN
 	if len(changeSet.ChangeSet) == 0 {
-		ingestionLog.State = reconcilerpb.State_NO_CHANGES
-		if err := o.repository.UpdateIngestionLogStateWithError(ctx, ingestionLogID, reconcilerpb.State_NO_CHANGES, nil); err != nil {
-			o.logger.Warn("failed to update ingestion log state (error ignored)", "ingestionLogID", ingestionLogID, "error", err)
-			// TODO(ltucker): This should be in a transaction.  Can leave an inconsistent state marked on the ingestion log.
-			// return nil, err
-		}
+		state = reconcilerpb.State_NO_CHANGES
 	}
 
-	ingestionLog.State = reconcilerpb.State_OPEN
-	if err := o.repository.UpdateIngestionLogStateWithError(ctx, ingestionLogID, reconcilerpb.State_OPEN, nil); err != nil {
+	ingestionLog.State = state
+
+	if err := o.repository.UpdateIngestionLogStateWithError(ctx, ingestionLogID, state, nil); err != nil {
 		o.logger.Warn("failed to update ingestion log state (error ignored)", "ingestionLogID", ingestionLogID, "error", err)
 		// TODO(ltucker): This should be in a transaction.  Can leave an inconsistent state marked on the ingestion log.
 		// return nil, err