Merge pull request #436 from ivanauth/fix/skip-migrations-deployment-hash

fix: skipMigrations deployments shouldn't claim the real migration hash

Author: ecordell

pkg/config/config.go

@@ -1002,10 +1002,20 @@ func (c *Config) Deployment(migrationHash, secretHash string) *applyappsv1.Deplo
 	}
 
 	// ensure patches don't overwrite anything critical for operator function
+	//
+	// When migrations are skipped the deployment must not claim the real
+	// migration hash: check_migrations treats a deployment carrying the target
+	// hash as proof migrations already ran, so stamping the real hash here
+	// (overriding unpatchedDeployment's "skipped") causes a later
+	// migrations-required reconcile to skip the migration job entirely.
+	migrationAnnotation := migrationHash
+	if c.SkipMigrations {
+		migrationAnnotation = "skipped"
+	}
 	d.WithName(name).WithNamespace(c.Namespace).WithOwnerReferences(c.ownerRef()).
 		WithLabels(metadata.LabelsForComponent(c.Name, metadata.ComponentSpiceDBLabelValue)).
 		WithAnnotations(map[string]string{
-			metadata.SpiceDBMigrationRequirementsKey: migrationHash,
+			metadata.SpiceDBMigrationRequirementsKey: migrationAnnotation,
 		})
 	d.Spec.Selector.WithMatchLabels(map[string]string{metadata.KubernetesInstanceLabelKey: name})
 	d.Spec.Template.

pkg/config/config_test.go

@@ -2654,6 +2654,30 @@ func TestDeployment(t *testing.T) {
 		secret         *corev1.Secret
 		wantDeployment *applyappsv1.DeploymentApplyConfiguration
 	}{
+		{
+			name: "migration hash is preserved when migrations are not skipped",
+			cluster: v1alpha1.ClusterSpec{
+				SecretRef: "test-secret",
+				Config: json.RawMessage(`
+					{
+						"logLevel": "debug",
+						"datastoreEngine": "cockroachdb"
+					}
+				`),
+			},
+			secret: &corev1.Secret{Data: map[string][]byte{
+				"datastore_uri": []byte("uri"),
+				"preshared_key": []byte("psk"),
+			}},
+			wantDeployment: expectedDeployment(func(dep *applyappsv1.DeploymentApplyConfiguration) {
+				// migrations not skipped: the deployment must carry the real
+				// migration hash so check_migrations can tell it apart from a
+				// "skipped" deployment.
+				dep.WithAnnotations(map[string]string{
+					metadata.SpiceDBMigrationRequirementsKey: "1",
+				})
+			}),
+		},
 		{
 			name: "container name back compat: smp patch with old name",
 			cluster: v1alpha1.ClusterSpec{
@@ -3734,7 +3758,10 @@ func expectedDeployment(apply ...func(dep *applyappsv1.DeploymentApplyConfigurat
 			metadata.KubernetesVersionLabelKey:   "v1",
 		}).
 		WithAnnotations(map[string]string{
-			metadata.SpiceDBMigrationRequirementsKey: "1",
+			// every TestDeployment case below sets skipMigrations: true, so the
+			// deployment must advertise "skipped" rather than the real migration
+			// hash (otherwise check_migrations would treat it as already migrated).
+			metadata.SpiceDBMigrationRequirementsKey: "skipped",
 		}).
 		WithOwnerReferences(applymetav1.OwnerReference().
 			WithName("test").