Skip to content

Commit 4f17de3

Browse files
momesginMo Mesgin
momesgin
and
Mo Mesgin
authored
Fix chart targetNamespace references to use version annotations (#17604)
* Fix remaining chart target namespace references to use version annotations * fix minor styling * fix having no fallback for version namespace lookup failure --------- Co-authored-by: Mo Mesgin <mmesgin@Mos-M2-MacBook-Pro.local>
1 parent ef26907 commit 4f17de3

5 files changed

Lines changed: 133 additions & 7 deletions

File tree

‎shell/chart/monitoring/index.vue‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import Tabbed from '@shell/components/Tabbed';
1717
import { allHash } from '@shell/utils/promise';
1818
import { STORAGE_CLASS, PVC, SECRET, WORKLOAD_TYPES } from '@shell/config/types';
1919
import { CATTLE_MONITORING_NAMESPACE } from '@shell/utils/monitoring';
20+
import { CATALOG as CATALOG_ANNOTATIONS } from '@shell/config/labels-annotations';
2021
2122
export default {
2223
emits: ['register-before-hook', 'input'],
@@ -50,6 +51,12 @@ export default {
5051
return {};
5152
},
5253
},
54+
55+
// The selected chart version object, passed from install.vue
56+
version: {
57+
type: Object,
58+
default: null,
59+
},
5360
},
5461
5562
async fetch() {
@@ -92,7 +99,9 @@ export default {
9299
93100
const hash = await allHash(hashPromises);
94101
95-
this.targetNamespace = hash.namespaces[this.chart.targetNamespace] || false;
102+
const ns = this.version?.annotations?.[CATALOG_ANNOTATIONS.NAMESPACE] || this.chart.targetNamespace;
103+
104+
this.targetNamespace = hash.namespaces[ns] || false;
96105
97106
if (!isEmpty(hash.storageClasses)) {
98107
this.storageClasses = hash.storageClasses;

‎shell/components/InstallHelmCharts.vue‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -248,7 +248,7 @@ export default {
248248
249249
};
250250
251-
this.installCmd.namespace = this.targetNamespace || this.chart?.targetNamespace || 'default';
251+
this.installCmd.namespace = this.targetNamespace || this.versionInfo?.chart?.annotations?.[CATALOG_ANNOTATIONS.NAMESPACE] || this.chart?.targetNamespace || 'default';
252252
}
253253
},
254254
@@ -360,7 +360,7 @@ export default {
360360
this.canInstallChart = false;
361361
} else {
362362
try {
363-
const app = await this.$store.dispatch(`${ this.store }/find`, { type: CATALOG.APP, id: `${ this.targetNamespace || this.chart?.targetNamespace || 'default' }/${ this.chartName }` });
363+
const app = await this.$store.dispatch(`${ this.store }/find`, { type: CATALOG.APP, id: `${ this.targetNamespace || this.versionInfo?.chart?.annotations?.[CATALOG_ANNOTATIONS.NAMESPACE] || this.chart?.targetNamespace || 'default' }/${ this.chartName }` });
364364
365365
if (app) {
366366
this.installedVersion = app?.spec?.chart?.metadata?.appVersion;

‎shell/mixins/__tests__/chart.test.ts‎

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -214,6 +214,72 @@ describe('chartMixin', () => {
214214
id: 'custom-ns/custom-name'
215215
});
216216
});
217+
218+
it('should fall back to matchingInstalledApps when version namespace lookup fails', async() => {
219+
const installedApp = {
220+
id: 'other-ns/custom-name',
221+
spec: { chart: { metadata: { version: '0.9.0' } } }
222+
};
223+
224+
const mockStore = {
225+
dispatch: jest.fn((action, payload) => {
226+
if (action === 'cluster/find') {
227+
return Promise.reject(new Error('not found'));
228+
}
229+
230+
return Promise.resolve();
231+
}),
232+
getters: {
233+
currentCluster: () => {},
234+
isRancher: () => true,
235+
'catalog/repo': () => {
236+
return () => 'repo';
237+
},
238+
'catalog/chart': () => {
239+
return {
240+
id: 'chart-id',
241+
versions: [{ version: '1.0.0' }],
242+
matchingInstalledApps: [installedApp]
243+
};
244+
},
245+
'catalog/version': () => ({
246+
version: '1.0.0',
247+
annotations: {
248+
[CATALOG_ANNOTATIONS.NAMESPACE]: 'custom-ns',
249+
[CATALOG_ANNOTATIONS.RELEASE_NAME]: 'custom-name',
250+
}
251+
}),
252+
'prefs/get': () => {},
253+
'i18n/t': () => jest.fn()
254+
}
255+
};
256+
257+
const DummyComponent = {
258+
mixins: [ChartMixin],
259+
template: '<div></div>',
260+
};
261+
262+
const wrapper = mount(
263+
DummyComponent,
264+
{
265+
global: {
266+
mocks: {
267+
$store: mockStore,
268+
$route: {
269+
query: {
270+
chart: 'chart_name',
271+
versionName: '1.0.0'
272+
}
273+
}
274+
}
275+
}
276+
});
277+
278+
await wrapper.vm.fetchChart();
279+
280+
expect(wrapper.vm.existing).toStrictEqual(installedApp);
281+
expect(wrapper.vm.mode).toStrictEqual('edit');
282+
});
217283
});
218284

219285
describe('action', () => {
@@ -366,6 +432,52 @@ describe('chartMixin', () => {
366432
});
367433
});
368434

435+
describe('isChartTargeted', () => {
436+
const DummyComponent = {
437+
mixins: [ChartMixin],
438+
template: '<div></div>',
439+
};
440+
441+
const mockStore = {
442+
dispatch: jest.fn(() => Promise.resolve()),
443+
getters: { 'i18n/t': (key: string) => key }
444+
};
445+
446+
it('should return truthy when version annotations have both namespace and release-name', () => {
447+
const wrapper = mount(DummyComponent, {
448+
data: () => ({
449+
version: {
450+
annotations: {
451+
[CATALOG_ANNOTATIONS.NAMESPACE]: 'custom-ns',
452+
[CATALOG_ANNOTATIONS.RELEASE_NAME]: 'custom-name',
453+
}
454+
}
455+
}),
456+
global: { mocks: { $store: mockStore } }
457+
});
458+
459+
expect(wrapper.vm.isChartTargeted).toBeTruthy();
460+
});
461+
462+
it('should return falsy when version annotations are missing', () => {
463+
const wrapper = mount(DummyComponent, {
464+
data: () => ({ version: { annotations: {} } }),
465+
global: { mocks: { $store: mockStore } }
466+
});
467+
468+
expect(wrapper.vm.isChartTargeted).toBeFalsy();
469+
});
470+
471+
it('should return falsy when version is null', () => {
472+
const wrapper = mount(DummyComponent, {
473+
data: () => ({ version: null }),
474+
global: { mocks: { $store: mockStore } }
475+
});
476+
477+
expect(wrapper.vm.isChartTargeted).toBeFalsy();
478+
});
479+
});
480+
369481
describe('mappedVersions', () => {
370482
it('should return versions sorted by semver (descending)', () => {
371483
const versions = [

‎shell/mixins/chart.js‎

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -271,7 +271,7 @@ export default {
271271
},
272272

273273
isChartTargeted() {
274-
return this.chart?.targetNamespace && this.chart?.targetName;
274+
return this.version?.annotations?.[CATALOG_ANNOTATIONS.NAMESPACE] && this.version?.annotations?.[CATALOG_ANNOTATIONS.RELEASE_NAME];
275275
},
276276

277277
hasQuestions() {
@@ -423,8 +423,12 @@ export default {
423423
});
424424
this.mode = _EDIT;
425425
} catch (e) {
426-
this.mode = _CREATE;
427-
this.existing = null;
426+
// Version targets a different namespace than where the app is installed.
427+
// Fall back to matching installed apps (e.g. chart detail page).
428+
const matching = this.chart?.matchingInstalledApps || [];
429+
430+
this.existing = matching[0] || null;
431+
this.mode = this.existing ? _EDIT : _CREATE;
428432
}
429433
} else {
430434
// Regular chart (not targeted) - check if there are installed instances.

‎shell/pages/c/_cluster/apps/charts/chart.vue‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -859,7 +859,6 @@ export default {
859859
.readme-wrapper {
860860
position: relative;
861861
flex: 1;
862-
min-width: 400px;
863862
864863
.open-readme-button {
865864
display: flex;
@@ -881,6 +880,8 @@ export default {
881880
}
882881
883882
&__readme {
883+
flex: 1;
884+
min-width: 400px;
884885
padding: 12px 24px;
885886
}
886887
&__info {

0 commit comments

Comments
 (0)