first pass on merging eslint rules #2914

rsun19 · 2024-06-13T21:44:44Z

Partially Closes: RHOAIENG-2534

Description

Added frontend eslint and tsconfig rules to the backend, as well as package.json packages.

You can see the added rules here

How Has This Been Tested?

cd backend && npm run test
npm run build

Test Impact

I have not implemented functionality, just refactored code.

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work (ran npm run test)
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit or cypress tests for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

codecov · 2024-06-14T20:50:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.52%. Comparing base (0c6256d) to head (a2e33eb).

❗ Current head a2e33eb differs from pull request most recent head ecbd421

Please upload reports for the commit ecbd421 to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2914      +/-   ##
==========================================
- Coverage   79.36%   78.52%   -0.85%     
==========================================
  Files        1139     1139              
  Lines       24284    24171     -113     
  Branches     6186     6099      -87     
==========================================
- Hits        19273    18980     -293     
- Misses       5011     5191     +180

see 142 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0c6256d...ecbd421. Read the comment docs.

rsun19 · 2024-06-17T14:44:23Z

/retest

backend/src/plugins/kube.ts

backend/src/routes/api/accelerator-profiles/acceleratorProfilesUtils.ts

christianvogt · 2024-06-18T20:47:10Z

backend/src/routes/api/accelerator-profiles/acceleratorProfilesUtils.ts

-  const customObjectsApi = fastify.kube.customObjectsApi;
-  const namespace = fastify.kube.namespace;
+  if (typeof fastify.kube === 'undefined') {
+    throw new Error('fastify.kube is not defined.');
+  }
+  const { customObjectsApi } = fastify.kube;
+  const { namespace } = fastify.kube;


I feel like kube should always be defined since we own the plugin and install it always.
Fix KubeFastifyInstance

One problem is that lots of functions take in FastifyInstance as the parameter like: export default fp(async (fastify: FastifyInstance) => {. However, when it tries to call a function requiring KubeFastifyInstance, there's a type mismatch now that kube is required. How should this be dealt with? Should I implement a typeguard essentially checking if kube is present like 'kube' in fastify?

Error:

src/plugins/kube.ts:81:30 - error TS2345: Argument of type 'FastifyInstance<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, FastifyBaseLogger, FastifyTypeProviderDefault>' is not assignable to parameter of type 'KubeFastifyInstance'. Property 'kube' is missing in type 'FastifyInstance<RawServerDefault, IncomingMessage, ServerResponse<IncomingMessage>, FastifyBaseLogger, FastifyTypeProviderDefault>' but required in type '{ kube: KubeDecorator; }'. 81 initializeWatchedResources(fastify);

Ok, i fixed this by changing the parameter types. Don't know if that's how it should be done though.

backend/src/routes/api/builds/list.ts

backend/src/routes/api/prometheus/index.ts

backend/src/utils/route-security.ts

backend/src/routes/api/accelerator-profiles/acceleratorProfilesUtils.ts

backend/src/routes/api/cluster-settings/clusterSettingsUtils.ts

backend/src/routes/api/components/list.ts

backend/src/utils/resourceWatcher.ts

backend/src/utils/userUtils.ts

christianvogt · 2024-06-26T21:39:39Z

backend/src/utils/userUtils.ts

+      if (
+        typeof e === 'object' &&
+        e !== null &&
+        'message' in e &&
+        typeof e.message === 'string' &&
+        'statusCode' in e &&
+        typeof e.statusCode === 'number'
+      ) {


I believe this can use createError.isHttpError

Created a new method instead for this instance, since Cannot find name 'createError'.ts(2304)

backend/src/routes/api/status/adminAllowedUsers.ts

backend/src/utils.ts

backend/src/utils/userUtils.ts

christianvogt · 2024-07-03T19:42:19Z

backend/src/utils/userUtils.ts

+    if (isHttpError(e)) {
+      const error = createCustomError(
+        e.message,
+        `Error getting Oauth Info for user, ${errorHandler(e)}`,
+        e.statusCode,
+      );
+      throw error;
+    } else {
+      throw new Error(`Error getting Oauth Info for user, ${errorHandler(e)}`);
+    }


Can probably default to status code 401:

Suggested change

if (isHttpError(e)) {

const error = createCustomError(

e.message,

`Error getting Oauth Info for user, ${errorHandler(e)}`,

e.statusCode,

);

throw error;

} else {

throw new Error(`Error getting Oauth Info for user, ${errorHandler(e)}`);

}

const error = createCustomError(

e.message,

`Error getting Oauth Info for user, ${errorHandler(e)}`,

(isHttpError(e) && e.statusCode) || 401,

);

throw error;

replaced e.message with errorHandler(e), since e might be undefined

backend/src/routes/api/accelerator-profiles/acceleratorProfilesUtils.ts

backend/src/routes/api/cluster-settings/clusterSettingsUtils.ts

backend/src/routes/api/components/list.ts

backend/src/types.ts

christianvogt · 2024-07-05T18:42:04Z

backend/src/utils/route-security.ts

-  if (![notebookNamespace, dashboardNamespace, MODEL_REGISTRY_NAMESPACE].includes(namespace)) {
+  if (![notebookNamespace, dashboardNamespace].includes(namespace)) {


Why is MODEL_REGISTRY_NAMESPACE removed?

I have no idea how that happened

christianvogt · 2024-07-10T18:49:27Z

/lgtm
/approve

did a bunch of ad hoc testing of various parts of the UI

openshift-ci · 2024-07-10T18:49:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

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~~ [christianvogt]

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

openshift-ci bot requested review from alexcreasy and lucferbux June 13, 2024 21:44

openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 13, 2024

rsun19 changed the title ~~first pass on merging eslint rules~~ [WIP] first pass on merging eslint rules Jun 13, 2024

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jun 13, 2024

rsun19 force-pushed the merge-eslint-v2 branch 2 times, most recently from e880400 to 0f44a1a Compare June 14, 2024 20:16

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 14, 2024

rsun19 changed the title ~~[WIP] first pass on merging eslint rules~~ first pass on merging eslint rules Jun 14, 2024

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jun 14, 2024

rsun19 changed the title ~~first pass on merging eslint rules~~ [WIP] first pass on merging eslint rules Jun 14, 2024

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jun 14, 2024

rsun19 force-pushed the merge-eslint-v2 branch from 0f44a1a to dbc66dd Compare June 14, 2024 20:40

rsun19 changed the title ~~[WIP] first pass on merging eslint rules~~ first pass on merging eslint rules Jun 14, 2024

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jun 14, 2024

rsun19 changed the title ~~first pass on merging eslint rules~~ [WIP] first pass on merging eslint rules Jun 17, 2024

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jun 17, 2024

rsun19 force-pushed the merge-eslint-v2 branch from dbc66dd to 475d092 Compare June 17, 2024 14:02

rsun19 changed the title ~~[WIP] first pass on merging eslint rules~~ first pass on merging eslint rules Jun 17, 2024

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jun 17, 2024

rsun19 force-pushed the merge-eslint-v2 branch from 475d092 to a2e33eb Compare June 18, 2024 13:55

christianvogt reviewed Jun 18, 2024

View reviewed changes

christianvogt reviewed Jun 19, 2024

View reviewed changes

backend/src/utils/route-security.ts Outdated Show resolved Hide resolved

backend/src/utils/route-security.ts Outdated Show resolved Hide resolved

rsun19 force-pushed the merge-eslint-v2 branch from a2e33eb to 332856c Compare June 20, 2024 19:51

rsun19 changed the title ~~first pass on merging eslint rules~~ [WIP] first pass on merging eslint rules Jun 20, 2024

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Jun 20, 2024

rsun19 force-pushed the merge-eslint-v2 branch 2 times, most recently from 87efeef to d26e8dc Compare June 21, 2024 17:14

rsun19 force-pushed the merge-eslint-v2 branch from d26e8dc to 387c86e Compare June 21, 2024 18:54

rsun19 changed the title ~~[WIP] first pass on merging eslint rules~~ first pass on merging eslint rules Jun 25, 2024

openshift-ci bot removed the do-not-merge/work-in-progress This PR is in WIP state label Jun 25, 2024

christianvogt reviewed Jun 26, 2024

View reviewed changes

rsun19 force-pushed the merge-eslint-v2 branch from 387c86e to fb25d3f Compare June 28, 2024 20:59

openshift-merge-robot added the needs-rebase PR needs to be rebased label Jun 28, 2024

rsun19 force-pushed the merge-eslint-v2 branch from fb25d3f to 2ba750e Compare June 28, 2024 21:04

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Jun 28, 2024

rsun19 force-pushed the merge-eslint-v2 branch from 2ba750e to ecbd421 Compare June 28, 2024 21:20

christianvogt reviewed Jul 3, 2024

View reviewed changes

rsun19 force-pushed the merge-eslint-v2 branch from ecbd421 to 9403354 Compare July 5, 2024 15:44

christianvogt reviewed Jul 5, 2024

View reviewed changes

rsun19 force-pushed the merge-eslint-v2 branch from 9403354 to e50852a Compare July 5, 2024 19:33

first pass on merging eslint rules

bb4e9df

rsun19 force-pushed the merge-eslint-v2 branch from e50852a to bb4e9df Compare July 5, 2024 19:35

openshift-ci bot assigned christianvogt Jul 10, 2024

openshift-ci bot added the lgtm label Jul 10, 2024

openshift-ci bot added the approved label Jul 10, 2024

openshift-merge-bot bot merged commit 98a3bbf into opendatahub-io:main Jul 10, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

first pass on merging eslint rules #2914

first pass on merging eslint rules #2914

rsun19 commented Jun 13, 2024 •

edited by christianvogt

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

rsun19 commented Jun 17, 2024

christianvogt Jun 18, 2024

rsun19 Jun 20, 2024 •

edited

Loading

rsun19 Jun 21, 2024

christianvogt Jun 26, 2024

rsun19 Jun 28, 2024

christianvogt Jul 3, 2024

rsun19 Jul 5, 2024

christianvogt Jul 5, 2024

rsun19 Jul 5, 2024

christianvogt commented Jul 10, 2024

openshift-ci bot commented Jul 10, 2024

		if (![notebookNamespace, dashboardNamespace, MODEL_REGISTRY_NAMESPACE].includes(namespace)) {
		if (![notebookNamespace, dashboardNamespace].includes(namespace)) {

first pass on merging eslint rules #2914

first pass on merging eslint rules #2914

Conversation

rsun19 commented Jun 13, 2024 • edited by christianvogt Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

codecov bot commented Jun 14, 2024 • edited Loading

Codecov Report

rsun19 commented Jun 17, 2024

Choose a reason for hiding this comment

rsun19 Jun 20, 2024 • edited Loading

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

christianvogt commented Jul 10, 2024

openshift-ci bot commented Jul 10, 2024

rsun19 commented Jun 13, 2024 •

edited by christianvogt

Loading

codecov bot commented Jun 14, 2024 •

edited

Loading

rsun19 Jun 20, 2024 •

edited

Loading