diff --git a/.changeset/yellow-ties-switch.md b/.changeset/yellow-ties-switch.md new file mode 100644 index 0000000000..784d2a18ae --- /dev/null +++ b/.changeset/yellow-ties-switch.md @@ -0,0 +1,5 @@ +--- +'@backstage/catalog-model': minor +--- + +The catalog no longer attempts to merge old and new annotations, when updating an entity from a remote location. This was a behavior that was copied from kubernetes, and catered to use cases where you wanted to use HTTP POST to update an entity in-place, outside of what the refresh loop does. This has proved to be a mistake, because as a side effect, the refresh loop effectively is unable to ever delete annotations when they are removed from source YAML. This is obviously a breaking change, but we believe that this is not a behavior that is relied upon in the wild, and it has never been an actually supported use flow of the catalog. We therefore choose to break the behavior outright, and instead just store updated annotations verbatim - just like we already do for example for labels diff --git a/packages/catalog-model/src/entity/util.test.ts b/packages/catalog-model/src/entity/util.test.ts index e4399bbe05..c7e2c036b5 100644 --- a/packages/catalog-model/src/entity/util.test.ts +++ b/packages/catalog-model/src/entity/util.test.ts @@ -96,18 +96,12 @@ describe('util', () => { b = lodash.cloneDeep(a); b.metadata.labels.labelKey += 'a'; expect(entityHasChanges(a, b)).toBe(true); - }); - - it('detects annotation changes, but not removals', () => { - let b: any = lodash.cloneDeep(a); + b = lodash.cloneDeep(a); b.metadata.annotations.annotationKey += 'a'; expect(entityHasChanges(a, b)).toBe(true); b = lodash.cloneDeep(a); - b.metadata.annotations.n = 'n'; - expect(entityHasChanges(a, b)).toBe(true); - b = lodash.cloneDeep(a); delete b.metadata.annotations.annotationKey; - expect(entityHasChanges(a, b)).toBe(false); + expect(entityHasChanges(a, b)).toBe(true); }); it('detects spec changes', () => { diff --git a/packages/catalog-model/src/entity/util.ts b/packages/catalog-model/src/entity/util.ts index ed68339a99..2c63cc4c49 100644 --- a/packages/catalog-model/src/entity/util.ts +++ b/packages/catalog-model/src/entity/util.ts @@ -54,10 +54,6 @@ export function generateEntityEtag(): string { * @param next The new state of the entity */ export function entityHasChanges(previous: Entity, next: Entity): boolean { - if (entityHasAnnotationChanges(previous, next)) { - return true; - } - const e1 = lodash.cloneDeep(previous); const e2 = lodash.cloneDeep(next); @@ -67,6 +63,18 @@ export function entityHasChanges(previous: Entity, next: Entity): boolean { if (!e2.metadata.labels) { e2.metadata.labels = {}; } + if (!e1.metadata.annotations) { + e1.metadata.annotations = {}; + } + if (!e2.metadata.annotations) { + e2.metadata.annotations = {}; + } + if (!e1.metadata.tags) { + e1.metadata.tags = []; + } + if (!e2.metadata.tags) { + e2.metadata.tags = []; + } // Remove generated fields delete e1.metadata.uid; @@ -76,10 +84,6 @@ export function entityHasChanges(previous: Entity, next: Entity): boolean { delete e2.metadata.etag; delete e2.metadata.generation; - // Remove already compared things - delete e1.metadata.annotations; - delete e2.metadata.annotations; - // Remove things that we explicitly do not compare delete e1.relations; delete e2.relations; @@ -106,14 +110,6 @@ export function generateUpdatedEntity(previous: Entity, next: Entity): Entity { const result = lodash.cloneDeep(next); - // Annotations are merged, with the new ones taking precedence - if (previous.metadata.annotations) { - next.metadata.annotations = { - ...previous.metadata.annotations, - ...next.metadata.annotations, - }; - } - // Generated fields are copied and updated const bumpEtag = entityHasChanges(previous, result); const bumpGeneration = !lodash.isEqual(previous.spec, result.spec); @@ -123,26 +119,3 @@ export function generateUpdatedEntity(previous: Entity, next: Entity): Entity { return result; } - -function entityHasAnnotationChanges(previous: Entity, next: Entity): boolean { - // Since the next annotations get merged into the previous, extract only - // the overlapping keys and check if their values match. - if (next.metadata.annotations) { - if (!previous.metadata.annotations) { - return true; - } - if ( - !lodash.isEqual( - next.metadata.annotations, - lodash.pick( - previous.metadata.annotations, - Object.keys(next.metadata.annotations), - ), - ) - ) { - return true; - } - } - - return false; -}