chore(quality): add scripts for code quality #43

andrewazores · 2025-01-10T20:24:15Z

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Read the contributing guidelines
Linked a relevant issue which this PR resolves
Linked any other relevant issues, PR's, or documentation, if any
Resolved all conflicts, if any
Rebased your branch PR on top of the latest upstream main branch
Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main

See #14

Description of the change:

Adds some scripts and configurations for formatting, license headers, linting, etc. See https://github.com/cryostatio/cryostat-web for the references where most of these things were copied from.
Adds CI jobs to run some of these checks separately, since they may not be run as part of the container image build.

How to manually test:

Use https://github.com/nektos/act . I use the gh CLI extension.
Check out this PR.
Run ex. $ act -W .github/workflows/ci.yaml -j type-check. Try this for each of the new CI jobs.
Try the package.json scripts directly, ex. yarn eslint:check or npm run license:check.
Check this PR's CI status. Not everything added here currently passes due to deficiencies in the project sources.

andrewazores · 2025-01-14T15:06:16Z

Done messing around, this is ready for review now.

aptmac · 2025-01-14T15:40:54Z

Looks good to me, there's just a couple of things being caught by the CI build.

The lint step can be cleaned up with the following diff. It's complaining about wanting to substitute quotes in the string, and then later complains about an unused variable that can just be fixed by updating the eslint and underscoring the variable:

diff --git a/.eslintrc.yml b/.eslintrc.yml
index 60ea3aa..800007b 100644
--- a/.eslintrc.yml
+++ b/.eslintrc.yml
@@ -19,6 +19,10 @@ plugins:
 rules:
   prettier/prettier:
     - error
+  "@typescript-eslint/no-unused-vars":
+  - warn
+  - argsIgnorePattern: "^_"
+
 settings:
   react:
     version: detect
diff --git a/src/openshift/pages/ExamplePage.tsx b/src/openshift/pages/ExamplePage.tsx
index d64f5f9..d8fe883 100644
--- a/src/openshift/pages/ExamplePage.tsx
+++ b/src/openshift/pages/ExamplePage.tsx
@@ -185,7 +185,8 @@ export default function ExamplePage() {
         <PageSection variant="light">
           {instance && instance.metadata ? (
             <Text>
-              Selected Cryostat "{instance.metadata.name}" in project "{instance.metadata.namespace}"
+              Selected Cryostat `&quot;`{instance.metadata.name}`&quot;` in project `&quot;`
+              {instance.metadata.namespace}`&quot;`
             </Text>
           ) : undefined}
           <Text>API Request Method</Text>
diff --git a/src/openshift/services/ApiService.tsx b/src/openshift/services/ApiService.tsx
index 28e97dc..0c774cd 100644
--- a/src/openshift/services/ApiService.tsx
+++ b/src/openshift/services/ApiService.tsx
@@ -34,7 +34,7 @@ export class ApiService {
     );
   }
 
-  cryostat(ns: string, name: string, method: string, requestPath: string, body?: object): Observable<string> {
+  cryostat(ns: string, name: string, method: string, requestPath: string, _body?: object): Observable<string> {
     const url = this.proxyUrl(`upstream/${requestPath}`);
     return from(
       consoleFetch(url.toString(), {

The type-check is complaining that it can't find the modules provided by the cryostat-web repo, probably because it doesn't look like the CI checks out or initializes the submodule.

Co-authored-by: aptmac <[email protected]>

andrewazores · 2025-01-14T15:55:29Z

Maybe for the type-check step we should have a separate tsconfig.json that only checks the sources for this plugin on its own, rather than also trying to analyze the submodule?

andrewazores · 2025-01-14T16:16:57Z

That still runs into an odd issue:

$ npx tsc --noEmit -p tsconfig-plugin.json
src/cryostat-web/src/app/About/About.tsx:17:26 - error TS2307: Cannot find module '@app/assets/cryostat_logo_hori_rgb_default.svg' or its corresponding type declarations.

17 import cryostatLogo from '@app/assets/cryostat_logo_hori_rgb_default.svg';
                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/cryostat-web/src/app/About/About.tsx:18:30 - error TS2307: Cannot find module '@app/assets/cryostat_logo_hori_rgb_reverse.svg' or its corresponding type declarations.

18 import cryostatLogoDark from '@app/assets/cryostat_logo_hori_rgb_reverse.svg';
                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/cryostat-web/src/app/Dashboard/utils.tsx:17:26 - error TS2307: Cannot find module '@app/assets/cryostat_icon_rgb_default.svg' or its corresponding type declarations.

17 import cryostatLogo from '@app/assets/cryostat_icon_rgb_default.svg';
                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/cryostat-web/src/app/Dashboard/utils.tsx:18:30 - error TS2307: Cannot find module '@app/assets/cryostat_icon_rgb_reverse.svg' or its corresponding type declarations.

18 import cryostatLogoDark from '@app/assets/cryostat_icon_rgb_reverse.svg';
                                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/cryostat-web/src/app/Topology/Actions/quicksearches/custom-target.tsx:16:24 - error TS2307: Cannot find module '@app/assets/openjdk.svg' or its corresponding type declarations.

16 import openjdkSvg from '@app/assets/openjdk.svg';
                          ~~~~~~~~~~~~~~~~~~~~~~~~~

src/cryostat-web/src/app/Topology/GraphView/CustomGroup.tsx:17:24 - error TS2307: Cannot find module '@app/assets/openjdk.svg' or its corresponding type declarations.

17 import openjdkSvg from '@app/assets/openjdk.svg';
                          ~~~~~~~~~~~~~~~~~~~~~~~~~

src/cryostat-web/src/app/Topology/GraphView/CustomNode.tsx:17:25 - error TS2307: Cannot find module '@app/assets/cryostat_icon_rgb_default.svg' or its corresponding type declarations.

17 import cryostatSvg from '@app/assets/cryostat_icon_rgb_default.svg';
                           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/cryostat-web/src/app/Topology/GraphView/CustomNode.tsx:18:24 - error TS2307: Cannot find module '@app/assets/openjdk.svg' or its corresponding type declarations.

18 import openjdkSvg from '@app/assets/openjdk.svg';
                          ~~~~~~~~~~~~~~~~~~~~~~~~~


Found 8 errors in 5 files.

Errors  Files
     2  src/cryostat-web/src/app/About/About.tsx:17
     2  src/cryostat-web/src/app/Dashboard/utils.tsx:17
     1  src/cryostat-web/src/app/Topology/Actions/quicksearches/custom-target.tsx:16
     1  src/cryostat-web/src/app/Topology/GraphView/CustomGroup.tsx:17
     2  src/cryostat-web/src/app/Topology/GraphView/CustomNode.tsx:17

andrewazores · 2025-01-14T16:26:09Z

Not sure what's wrong with the type-check action now. It works as expected locally and when running in a fresh container with act.

aptmac · 2025-01-15T00:48:52Z

Edit: Hm, nevermind, we're stuck at react-i18next ^11.7.3 for the plugin because of the Console. It looks like there's a difference between TFunction as exported by react-18next v11.7.3 and i18next. Also TFunction isn't exported by react-18next v12 and above.

Edit 2: If I downgrade the react-i18next version in cryostat-web to the one that matches the Console (11.7.3), and then change the TFunction imports to get from react-i18next instead of i18next, then these errors go away for me locally. I have a branch here that I've been trying with: aptmac/cryostat-web@857b733

Original: It gets cleared up when dependencies are added. The errors about react-router and react-router-dom were similar to the errors I would hit prior to the cryostat-web react-router-dom-v5-compat commit that just went in; updating the submodule to bring in the latest changes fixes those errors.

The other errors have to do with not finding i18next, which isn't explicitly listed as a devDependency so after adding that to the package.json we get a new error: Error: src/cryostat-web/src/app/Archives/AllTargetsArchivedRecordingsTable.tsx(347,49): error TS2345: Argument of type 'TFunction<string, undefined>' is not assignable to parameter of type 'TFunction<"translation", undefined>'.

This gets resolved by updating react-i18next. I tried just bumping it to the version that cryostat-web uses and the CI runs started coming back okay.

Passing CI run: https://github.com/aptmac/cryostat-openshift-console-plugin/actions/runs/12779083328/job/35623104990

Diff: 0001-try-fixing-errors-on-pr43.txt

andrewazores requested review from a team as code owners January 10, 2025 20:24

andrewazores added chore Refactor, rename, cleanup, etc. ci labels Jan 10, 2025

andrewazores force-pushed the codequality branch from d38d825 to f52fd76 Compare January 10, 2025 20:27

andrewazores requested a review from aptmac January 10, 2025 20:46

andrewazores enabled auto-merge January 10, 2025 20:50

andrewazores disabled auto-merge January 10, 2025 21:51

andrewazores force-pushed the codequality branch from 825ca43 to c5cc7c5 Compare January 10, 2025 21:52

andrewazores added 10 commits January 14, 2025 10:05

chore(quality): add scripts for code quality

cfca754

add configs

3190d6e

add licenses and apply formatting

717c0d8

add code quality check jobs

e940a88

remove

1dbb18c

lockfile

d2aa01d

fixup! chore(quality): add scripts for code quality

f6725a2

backend uses its own tsconfig

3c24578

concurrent lint checks

ac49835

lint

79ea6f4

andrewazores force-pushed the codequality branch from 978144e to 79ea6f4 Compare January 14, 2025 15:06

andrewazores and others added 3 commits January 14, 2025 10:45

fix(lint): apply fixes

e5ecfc8

Co-authored-by: aptmac <[email protected]>

ci(check): check out submodule

85a25b5

fixup! fix(lint): apply fixes

92e0622

andrewazores added 2 commits January 14, 2025 11:20

typecheck fixup

f69facc

ignore .d.ts for license check

4ad76a2

include custom.d.ts

29b0d52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(quality): add scripts for code quality #43

chore(quality): add scripts for code quality #43

andrewazores commented Jan 10, 2025 •

edited

Loading

andrewazores commented Jan 14, 2025

aptmac commented Jan 14, 2025

andrewazores commented Jan 14, 2025

andrewazores commented Jan 14, 2025

andrewazores commented Jan 14, 2025

aptmac commented Jan 15, 2025 •

edited

Loading

chore(quality): add scripts for code quality #43

Are you sure you want to change the base?

chore(quality): add scripts for code quality #43

Conversation

andrewazores commented Jan 10, 2025 • edited Loading

Welcome to Cryostat! 👋

Before contributing, make sure you have:

Description of the change:

How to manually test:

andrewazores commented Jan 14, 2025

aptmac commented Jan 14, 2025

andrewazores commented Jan 14, 2025

andrewazores commented Jan 14, 2025

andrewazores commented Jan 14, 2025

aptmac commented Jan 15, 2025 • edited Loading

andrewazores commented Jan 10, 2025 •

edited

Loading

aptmac commented Jan 15, 2025 •

edited

Loading