Skip to content

Commit 392cea1

Browse files
torchiaftorchiaf
authored
Merge pull request #14699 from torchiaf/backport-2.11-fix-yaml-conflicts
[2.11.4] Fix YAML 409 conflict errors
2 parents 0d2c76f + 09da30f commit 392cea1

File tree

9 files changed

+467
-26
lines changed

9 files changed

+467
-26
lines changed

‎shell/components/ResourceYaml.vue‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -234,7 +234,7 @@ export default {
234234
}
235235
236236
try {
237-
await this.value.saveYaml(yaml);
237+
await this.value.saveYaml(yaml, this.initialYaml);
238238
} catch (err) {
239239
return onError.call(this, err);
240240
}

‎shell/edit/configmap.vue‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
<script>
2+
import jsyaml from 'js-yaml';
23
import CreateEditView from '@shell/mixins/create-edit-view';
34
import CruResource from '@shell/components/CruResource';
45
import NameNsDescription from '@shell/components/form/NameNsDescription';
@@ -66,7 +67,9 @@ export default {
6667
const yaml = await this.$refs.cru.createResourceYaml(this.yamlModifiers);
6768
6869
try {
69-
await this.value.saveYaml(yaml);
70+
const initialYaml = jsyaml.dump(this.initialValue);
71+
72+
await this.value.saveYaml(yaml, initialYaml);
7073
this.done();
7174
} catch (err) {
7275
this.errors.push(err);

‎shell/mixins/create-edit-view/impl.js‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,16 @@ export default {
117117
// If they are resolved, return a false-y value
118118
// Else they can't be resolved, return an array of errors to show to the user.
119119
async conflict() {
120-
return await handleConflict(this.initialValue.toJSON(), this.value, this.liveValue, this.$store.getters, this.$store, this.storeOverride || this.$store.getters['currentStore'](this.value.type));
120+
return await handleConflict(
121+
this.initialValue,
122+
this.value,
123+
this.liveValue,
124+
{
125+
dispatch: this.$store.dispatch,
126+
getters: this.$store.getters
127+
},
128+
this.storeOverride || this.$store.getters['currentStore'](this.value.type)
129+
);
121130
},
122131

123132
async save(buttonDone, url, depth = 0) {

‎shell/models/cluster.x-k8s.io.machinedeployment.js‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,7 @@ export default class CapiMachineDeployment extends SteveModel {
125125
return;
126126
}
127127

128-
const initialValue = this.cluster.toJSON();
128+
const initialValue = this.cluster;
129129

130130
this.inClusterSpec.quantity += delta;
131131

@@ -145,7 +145,16 @@ export default class CapiMachineDeployment extends SteveModel {
145145
let errors = exceptionToErrorsArray(err);
146146

147147
if ( err.status === 409 && depth < 2 ) {
148-
const conflicts = await handleConflict(initialValue, value, liveModel, this.$rootGetters, { dispatch: this.$dispatch }, 'management');
148+
const conflicts = await handleConflict(
149+
initialValue,
150+
value,
151+
liveModel,
152+
{
153+
dispatch: this.$dispatch,
154+
getters: this.$rootGetters
155+
},
156+
'management'
157+
);
149158

150159
if ( conflicts === false ) {
151160
// It was automatically figured out, save again

‎shell/models/fleet.cattle.io.cluster.js‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -206,8 +206,8 @@ export default class FleetCluster extends SteveModel {
206206
return parsedLabels;
207207
}
208208

209-
async saveYaml(yaml) {
210-
await this._saveYaml(yaml);
209+
async saveYaml(yaml, initialYaml) {
210+
await this._saveYaml(yaml, initialYaml);
211211

212212
const parsed = jsyaml.load(yaml);
213213

‎shell/plugins/dashboard-store/__tests__/normalize.test.ts‎

Lines changed: 192 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,192 @@
1+
import { handleConflict } from '@shell/plugins/dashboard-store/normalize';
2+
3+
describe('fn: handleConflict', () => {
4+
let mockStore: any;
5+
let mockToJSON: any;
6+
7+
beforeEach(() => {
8+
jest.clearAllMocks();
9+
10+
mockStore = {
11+
dispatch: jest.fn((action, payload) => Promise.resolve(payload)),
12+
getters: { 'i18n/t': jest.fn((key, params) => `${ key } - ${ JSON.stringify(params) }`) },
13+
};
14+
15+
mockToJSON = jest.fn((x) => x);
16+
});
17+
18+
it('should resolve conflict and return false if no actual conflicts (integration)', async() => {
19+
const initialValue = {
20+
metadata: { resourceVersion: '1' },
21+
spec: {
22+
replicas: 1,
23+
containers: [{
24+
name: 'app',
25+
image: 'image1'
26+
}]
27+
}
28+
};
29+
// User modifies 'replicas'
30+
const value = {
31+
metadata: { resourceVersion: '1' },
32+
spec: {
33+
replicas: 2,
34+
containers: [{
35+
name: 'app',
36+
image: 'image1'
37+
}]
38+
}
39+
};
40+
// Server did not modify 'replicas', but changed something else (e.g., added a label)
41+
const liveValue = {
42+
metadata: {
43+
resourceVersion: '2',
44+
labels: { env: 'prod' }
45+
},
46+
spec: {
47+
replicas: 1, // Same replicas as initialValue, so no conflict with userChange on replicas
48+
containers: [{
49+
name: 'app',
50+
image: 'image1'
51+
}]
52+
}
53+
};
54+
55+
const result = await handleConflict(initialValue, value, liveValue, mockStore, 'namespace', mockToJSON);
56+
57+
// resourceVersion should be updated from liveValue
58+
expect(value.metadata.resourceVersion).toStrictEqual('2');
59+
// Background changes (labels added) should have been applied to 'value'
60+
expect((value.metadata as any).labels).toStrictEqual({ env: 'prod' });
61+
// User's change (replicas) should have remained, as there was no conflict
62+
expect(value.spec.replicas).toStrictEqual(2);
63+
64+
expect(result).toStrictEqual(false); // No conflict, save can continue
65+
});
66+
67+
it('should return conflict errors if actual conflicts exist (integration)', async() => {
68+
const initialValue = {
69+
metadata: { resourceVersion: '1' },
70+
spec: {
71+
replicas: 1,
72+
containers: [{
73+
name: 'app',
74+
image: 'image1'
75+
}]
76+
}
77+
};
78+
// User modifies 'replicas' to 2
79+
const value = {
80+
metadata: { resourceVersion: '1' },
81+
spec: {
82+
replicas: 2,
83+
containers: [{
84+
name: 'app',
85+
image: 'image1'
86+
}]
87+
}
88+
};
89+
// Server modified 'replicas' to 3
90+
const liveValue = {
91+
metadata: { resourceVersion: '2' },
92+
spec: {
93+
replicas: 3, // Conflict with user's modification
94+
containers: [{
95+
name: 'app',
96+
image: 'image1'
97+
}]
98+
}
99+
};
100+
101+
const result = await handleConflict(initialValue, value, liveValue, mockStore, 'namespace', mockToJSON);
102+
103+
// resourceVersion should be updated
104+
expect(value.metadata.resourceVersion).toStrictEqual('2');
105+
expect(value.spec.replicas).toStrictEqual(3);
106+
107+
// We expect an error message due to the conflict on 'spec.replicas'
108+
expect(result).toStrictEqual([
109+
'validation.conflict - {"fields":"spec.replicas","fieldCount":1}'
110+
]);
111+
expect(mockStore.getters['i18n/t']).toHaveBeenCalledWith('validation.conflict', {
112+
fields: 'spec.replicas',
113+
fieldCount: 1
114+
});
115+
});
116+
117+
it('should handle nested object changes and conflicts', async() => {
118+
const initialValue = {
119+
metadata: { resourceVersion: '1' },
120+
spec: {
121+
selector: {
122+
matchLabels: {
123+
app: 'nginx',
124+
env: 'dev'
125+
}
126+
}
127+
}
128+
};
129+
// User modifies 'env' to 'prod'
130+
const value = {
131+
metadata: { resourceVersion: '1' },
132+
spec: {
133+
selector: {
134+
matchLabels: {
135+
app: 'nginx',
136+
env: 'prod'
137+
}
138+
}
139+
}
140+
};
141+
// Live modifies 'app' to 'httpd'
142+
const liveValue = {
143+
metadata: { resourceVersion: '2' },
144+
spec: {
145+
selector: {
146+
matchLabels: {
147+
app: 'httpd',
148+
env: 'dev'
149+
}
150+
}
151+
}
152+
};
153+
154+
const result = await handleConflict(initialValue, value, liveValue, mockStore, 'namespace', mockToJSON);
155+
156+
expect(mockStore.dispatch).toHaveBeenCalledTimes(3);
157+
158+
// Should apply newest resource version
159+
expect(value.metadata.resourceVersion).toStrictEqual('2');
160+
161+
// Background change on 'app' should be applied
162+
expect(value.spec.selector.matchLabels.app).toStrictEqual('httpd');
163+
// User's change on 'env' should have remained
164+
expect(value.spec.selector.matchLabels.env).toStrictEqual('prod');
165+
166+
expect(result).toStrictEqual(false); // No direct conflict on a single field
167+
});
168+
169+
it('should detect add/remove conflicts (integration)', async() => {
170+
const initialValue = {
171+
metadata: { resourceVersion: '1' },
172+
spec: { ports: [{ port: 80 }, { port: 90 }] }
173+
};
174+
// Remove port 90 and add 443
175+
const value = {
176+
metadata: { resourceVersion: '1' },
177+
spec: { ports: [{ port: 80 }, { port: 443 }] }
178+
};
179+
// Remove port 80
180+
const liveValue = {
181+
metadata: { resourceVersion: '2' },
182+
spec: { ports: [{ port: 80 }, { port: 90 }] }
183+
};
184+
185+
await handleConflict(initialValue, value, liveValue, mockStore, 'myNamespace', mockToJSON);
186+
187+
expect(mockStore.dispatch).toHaveBeenCalledTimes(3);
188+
expect(value.metadata.resourceVersion).toStrictEqual('2');
189+
190+
expect(value.spec.ports).toStrictEqual([{ port: 80 }, { port: 443 }]);
191+
});
192+
});

0 commit comments

Comments
 (0)