App upgrade notification shown for incorrect app (corrected filters and added further tests) #10180

cnotv · 2023-12-18T18:30:10Z

Summary

Changes are the same of #9806, except that repoType, repoName, chartName are not mandatory anymore, as they will just return no charts if none of the criteria is provided.
If parameters are provided, only charts with all of them will be returned.

Occurred changes and/or fixed issues

Technical notes summary

Added tests for:

Chart match logic
- Check given only all matching parameters if provided for Epinio case.
- Check given any matching parameter for returning always charts if anything matches (reason this is reopened).
Chart install view version selection

Kubewarden installation

I have a 2 breaking errors when I try locally with master to install Kubewarden to reproduce the mentioned case:

Copy to clipboard says function does not exists
addChart says the repo is actually null

After setting conditional variables it works, although I keep it for another time, since my PR covered already more than needed and the error is on master as well (also just when running locally).

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

… own file

… add covering tests

…upgrade cases

cnotv · 2024-01-12T10:31:03Z

Also I noticed that we had already a talk about tests for Kubewarden here:
#9232

richard-cox

@jordojordo Can you validate kube warden side is fine? In the description Giuseppe mentions some kubewarden strageness

richard-cox · 2024-01-19T11:49:53Z

shell/types/catalog.ts

+  version: string,
+}
+
+export interface IChart {


The chart in our store represented by this interlaces comes from https://github.com/rancher/dashboard/blob/master/shell/store/catalog.js#L505. Via comment i think this should reference that, and vice versa

I am aware of the JS file which I'm not going to touch 😄
What are you suggesting about the interface? I did not get it.

This would just be to link the two together. So if someone updates the object they also know to update the interface, or want to know where the object is instantiated.

/* * ... * Created in `addChart` shell/store/catalog.js */ export interface IChart

// Instance of `IChart` obj = classify(ctx, {

Oh but I have absolutely no idea what do we have there, so I have just put what I know and then later someone will have to update just as I did, otherwise nobody would ever write anything, no?

That's fine, but can you still add the suggested comments?

yeah absolutely, just taking time :)

richard-cox · 2024-01-19T11:59:48Z

shell/types/catalog.ts

@@ -0,0 +1,30 @@
+export interface IChartVersions {


The object this interface represents in from the entries collection from catalog.cattle.io.clusterrepos.

I think this should be called something like IClusterRepoChart

The object IChart if for is constructed locally (https://github.com/rancher/dashboard/blob/master/shell/store/catalog.js#L505.). Suggest this one is called IDashboardChart.

That makes it cleared what each one is.

richard-cox · 2024-01-19T12:02:04Z

shell/types/catalog.ts

+  chartNameDisplay: string;
+}
+
+export interface IChartOptions {


Suggest this one be renamed to something like 'IFilterChartOptions` to differentiate it from other options to do with charts

richard-cox · 2024-01-19T13:10:58Z

shell/models/catalog.cattle.io.app.js

-    const newestChart = versions?.[0];
-    const newestVersion = newestChart?.version;
-
-    if ( !thisVersion || !newestVersion ) {


this check, for truthy currentChartVersion and newestVersion, has been lost. i noticed that compare now has

if (in1 === undefined && in2 === undefined) { return null; }

if that was meant for a replacement, would vote to keep compare untouched and to bring it back to this context (as compare is used in other places). So it would be

if (!currentChartVersion || !newestVersion) { return null; }

It fails if I keep as it is, I'll try to check what I can do.
Ideally would be convenient to have real data to understand better the suggestion.

richard-cox · 2024-01-19T13:14:23Z

shell/models/catalog.cattle.io.app.js

-    const newestChart = versions?.[0];
-    const newestVersion = newestChart?.version;
-
-    if ( !thisVersion || !newestVersion ) {


Apart from the check for undefined above, the other changes in upgradeAvailable are just tidying up? just want to be sure i haven't missed something

richard-cox · 2024-01-19T13:32:15Z

shell/models/catalog.cattle.io.app.js

@@ -40,69 +41,48 @@ export default class CatalogApp extends SteveModel {
  }

  matchingChart(includeHidden) {
-    const chart = this.spec?.chart;
-
-    if ( !chart ) {


this early exit has been lost

shell/models/__tests__/catalog.cattle.io.app.test.ts

richard-cox · 2024-01-19T13:45:12Z

shell/models/__tests__/catalog.cattle.io.app.test.ts

+    });
+
+    describe('as latest version', () => {
+      it('excluding pre-releases', () => {


the title of this test is wrong, it returns the success case where there is an upgrade available

richard-cox · 2024-01-19T13:46:13Z

shell/models/__tests__/catalog.cattle.io.app.test.ts

+        expect(catalogApp.upgradeAvailable).toStrictEqual(expectation);
+      });
+
+      it('if no version is available', () => {


this is the same as above (should it include catalogApp.spec = { chart: { metadata: { } } };?)

No it's not necessary for computation.

to check explicitly for a missing version it would need to get passed the check for a missing chart and spec. so by providing the object above the test should hit the code that it's aiming to check.

as it stands the test is the same as for if no chart and doesn't exercise anything different

but the test is passing, otherwise we are missing one and I did not realize it.

the two tests if no chart and if no version is available are exactly the same. both tests should provide and env such that only the condition referenced in the test title is exercised

richard-cox · 2024-01-19T13:53:07Z

shell/store/__tests__/catalog.test.ts

+          repoName:  'rancher-charts',
+        },
+        { chartName: 'rancher-alerting-drivers' },
+        { repoType: 'cluster' },


given only repo type or repo name i don't think we should match any chart in that repo. feels like the minimum would be the chart name

cnotv · 2024-01-19T15:16:59Z

@jordojordo Can you validate kube warden side is fine? In the description Giuseppe mentions some kubewarden strageness

It seems to crash just in case of lack of charts, so I omitted it.

jordojordo · 2024-01-19T15:42:46Z

The sections where I previously had trouble with the catalog/chart getter have been resolved as I've added the repoName and repoType to each instance of those calls.

addChart says the repo is actually null

I didn't notice any issues here.

Copy to clipboard says function does not exists

I did run into the issue with the $copyText method not existing where I have used the CopyCode component here. I'm not sure how that's related to this PR, it's possible this was created/fixed somewhere else.

copy-code.mp4

cnotv · 2024-09-05T14:18:38Z

Closing since is outdated and replaced by a new issue: #11465

github-actions bot assigned cnotv Dec 18, 2023

github-actions bot added this to the v2.8.next1 milestone Dec 18, 2023

cnotv marked this pull request as draft December 20, 2023 13:03

cnotv force-pushed the feature/9613-chart-upgrade-mixup branch 2 times, most recently from 313d3a8 to 4496e57 Compare January 10, 2024 17:40

cnotv added 13 commits January 10, 2024 18:41

Heavy code cleanup: move function to utils, config and types to their…

a81dcbd

… own file

Add fallback and tests for version comparison not retrieving values

75567ca

Fix refactoring

b40ad20

Add partial case of array filterBy

108f3c0

Replace multiple functions handling the same case with single getter;…

eefd147

… add covering tests

Add no parameter case to be correctly handled

6523cc3

Add case with different chart values, e.g. Epinio issue case

4ad7458

Prevent issues if retrieved version is undefined; Add unit tests for …

46dc031

…upgrade cases

Filter out charts if lack parameters

ca72059

Add parameter to the test log

6fca9c0

Allow to fetch all the charts and filter them afterwards

c08a1ab

Create tests for missing chart filtering cases

af79b1b

Add tests for chart install version automatic selection from query

4496e57

cnotv requested a review from richard-cox January 11, 2024 21:55

cnotv marked this pull request as ready for review January 11, 2024 21:55

richard-cox requested changes Jan 19, 2024

View reviewed changes

nwmac modified the milestones: v2.9.0, v2.10.0 Jul 1, 2024

This was referenced Sep 3, 2024

App upgrade notification shown for incorrect app #9613

Closed

Installed Apps must restrict its chart versions array to single clusterRepo #11465

Open

cnotv closed this Sep 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

App upgrade notification shown for incorrect app (corrected filters and added further tests) #10180

App upgrade notification shown for incorrect app (corrected filters and added further tests) #10180

cnotv commented Dec 18, 2023 •

edited

Loading

cnotv commented Jan 12, 2024

richard-cox left a comment

richard-cox Jan 19, 2024

cnotv Jan 19, 2024

richard-cox Jan 22, 2024

cnotv Jan 22, 2024

richard-cox Jan 22, 2024

cnotv Jan 22, 2024

richard-cox Jan 19, 2024

richard-cox Jan 19, 2024

richard-cox Jan 19, 2024

cnotv Jan 19, 2024

richard-cox Jan 19, 2024

richard-cox Jan 19, 2024

richard-cox Jan 19, 2024

richard-cox Jan 19, 2024

cnotv Jan 19, 2024

richard-cox Jan 22, 2024

cnotv Jan 22, 2024

richard-cox Jan 22, 2024

richard-cox Jan 19, 2024

cnotv commented Jan 19, 2024

jordojordo commented Jan 19, 2024 •

edited

Loading

cnotv commented Sep 5, 2024

App upgrade notification shown for incorrect app (corrected filters and added further tests) #10180

App upgrade notification shown for incorrect app (corrected filters and added further tests) #10180

Conversation

cnotv commented Dec 18, 2023 • edited Loading

Summary

Occurred changes and/or fixed issues

Technical notes summary

Kubewarden installation

Areas or cases that should be tested

Areas which could experience regressions

Screenshot/Video

cnotv commented Jan 12, 2024

richard-cox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cnotv commented Jan 19, 2024

jordojordo commented Jan 19, 2024 • edited Loading

cnotv commented Sep 5, 2024

cnotv commented Dec 18, 2023 •

edited

Loading

jordojordo commented Jan 19, 2024 •

edited

Loading