Enable static models when going trough available resources #134

karelhala · 2023-02-15T13:07:41Z

Description

Since we have now full control over api discovery we can enhance it with static models. We've identified a couple of resources which won't change that often and we don't have to watch for their changes.

JIRA

RHCLOUD-23118

openshift-ci · 2023-02-15T13:07:59Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: karelhala

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [karelhala]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

codecov · 2023-02-15T13:14:35Z

Codecov Report

Patch coverage: 100.00% and project coverage change: +1.93 🎉

Comparison is base (3f2e899) 30.52% compared to head (0872aac) 32.45%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   30.52%   32.45%   +1.93%     
==========================================
  Files          26       31       +5     
  Lines         462      487      +25     
  Branches       78       84       +6     
==========================================
+ Hits          141      158      +17     
- Misses        294      301       +7     
- Partials       27       28       +1

Impacted Files	Coverage Δ
frontend/src/Utils/api-discovery/getResources.ts	`100.00% <100.00%> (ø)`
...ls/api-discovery/staticModels/apps.openshift.io.ts	`100.00% <100.00%> (ø)`
...ntend/src/Utils/api-discovery/staticModels/apps.ts	`100.00% <100.00%> (ø)`
.../api-discovery/staticModels/config.openshift.io.ts	`100.00% <100.00%> (ø)`
...ntend/src/Utils/api-discovery/staticModels/core.ts	`100.00% <100.00%> (ø)`
...tend/src/Utils/api-discovery/staticModels/index.ts	`100.00% <100.00%> (ø)`

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

florkbr · 2023-02-16T15:51:09Z

frontend/src/Utils/api-discovery/getResources.test.ts

@@ -303,4 +302,10 @@ describe('batchResourcesRequest', () => {
      },
    ]);
  });
+
+  test('should not call commonFetch for static model', async () => {


👍 for tests!

florkbr · 2023-02-16T15:53:17Z

frontend/src/Utils/api-discovery/staticModels/index.ts

@@ -0,0 +1,11 @@
+import core from './core';
+import apps from './apps';
+import configOs from './config.openshift.io';


@karelhala NIT - these names are a bit odd looking... it might help if we capitalize both O and S as it's an abbreviation for openshift? Or perhaps just use the entire openshift qualification.

Yeah, good point.

florkbr · 2023-02-16T16:15:15Z

frontend/src/Utils/api-discovery/getResources.ts

@@ -35,7 +36,8 @@ export const defineModels = (list: APIResourceList): K8sModelCommon[] => {

 export const batchResourcesRequest = (batch: string[]): Promise<DiscoveryResources>[] => {
  return batch.map<Promise<DiscoveryResources>>(async (p: string) => {
-    const resourceList = await commonFetchJSON<APIResourceList>(p);
+    const [, staticresourceList] = Object.entries(staticAPIModels).find(([key]) => key === p) || [];
+    const resourceList = staticresourceList || (await commonFetchJSON<APIResourceList>(p));


@karelhala so it looks like there is no way to override these static models correct? Even if what we get back from the server is different than what we have stored as static models?

I wonder if we should favor the static models initially, then replace the models later on? What are your thoughts.

Nevermind. I read the logic above incorrectly - I see we will only do this for exact matches for those static resources.

florkbr · 2023-02-17T00:16:58Z

frontend/src/Utils/api-discovery/__mocks__/staticCore.ts

@@ -0,0 +1,354 @@
+export default [
+  {
+    allResources: [


@karelhala could you shed some light on which server you collected this list from? I'm assuming the main devsandbox instance - but we should clarify for the future.

@florkbr yeah, from the devsandbox from prod environment.

florkbr · 2023-02-17T00:20:05Z

frontend/src/Utils/api-discovery/staticModels/apps.ts

+      verbs: ['get', 'patch', 'update'],
+    },
+  ],
+} as APIResourceList;


@karelhala I know it's probably easier to use type assertions here, but could we update to use type annotations instead to avoid weakening our type checking?

Good point!

karelhala · 2023-02-22T19:28:12Z

/retest

florkbr · 2023-03-13T23:27:57Z

@karelhala I needed #138 to test this locally, otherwise I went through the flow of creating/viewing an app in stonesoup with this PR and everything works as expected (and I no longer see the requests for these core static models).

Looks good to me 👍

florkbr · 2023-03-13T23:29:49Z

/lgtm

openshift-ci · 2023-03-13T23:41:38Z

@karelhala: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

openshift-ci bot requested review from BlakeHolifield and fhlavac February 15, 2023 13:07

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 15, 2023

florkbr reviewed Feb 17, 2023

View reviewed changes

karelhala added 2 commits February 20, 2023 10:49

Enable static models when going trough available resources

c86549e

Use type anotation instead of weak type check

0872aac

karelhala force-pushed the enable-static-models branch from 351d5c7 to 0872aac Compare February 20, 2023 15:49

openshift-ci bot assigned florkbr Mar 13, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 13, 2023

openshift-merge-robot merged commit 00aff6b into openshift:main Mar 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable static models when going trough available resources #134

Enable static models when going trough available resources #134

karelhala commented Feb 15, 2023

openshift-ci bot commented Feb 15, 2023

codecov bot commented Feb 15, 2023 •

edited

florkbr Feb 16, 2023

florkbr Feb 16, 2023

karelhala Feb 20, 2023

florkbr Feb 16, 2023

florkbr Mar 13, 2023

florkbr Feb 17, 2023

karelhala Feb 20, 2023

florkbr Feb 17, 2023

karelhala Feb 20, 2023

karelhala commented Feb 22, 2023

florkbr commented Mar 13, 2023

florkbr commented Mar 13, 2023

openshift-ci bot commented Mar 13, 2023

Enable static models when going trough available resources #134

Enable static models when going trough available resources #134

Conversation

karelhala commented Feb 15, 2023

Description

JIRA

openshift-ci bot commented Feb 15, 2023

codecov bot commented Feb 15, 2023 • edited

Codecov Report

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

karelhala commented Feb 22, 2023

florkbr commented Mar 13, 2023

florkbr commented Mar 13, 2023

openshift-ci bot commented Mar 13, 2023

codecov bot commented Feb 15, 2023 •

edited