Skip to content

Conversation

@elaine-mattos
Copy link

@elaine-mattos elaine-mattos commented Jul 15, 2025

Hi dear clearlydefined community,

My team is currently evaluating the ClearlyDefined API for integration into our compliance workflows. While testing in our fork, we noticed several deprecated dependencies and decided to modernize the setup.

This PR upgrades the codebase to support Node.js 20 24 and updates a range of dependencies. Along the way, we made necessary refactors due to breaking changes, and introduced improvements in initialization and error handling.

We're still getting familiar with the codebase, so please feel free to comment or guide us if we’ve misunderstood anything. Also, if this PR feels too large, we’re happy to split it into smaller pieces.

Some details

  • Node and Dependency updates:
    • Upgraded project to Node.js 20 24;
    • Updated multiple dependencies to their latest compatible versions.
  • Express 5 migration:
    • Removed express-init in favor of direct async initialization;
    • Updated middleware and route initialization to match Express 5 syntax;
    • Changed app.options('*', cors()) to app.options('*splat', cors()) per new path syntax.
  • GitHub & GitLab API usage:
    • Updated all usages of @octokit/rest and @gitbeaker/node to match new APIs and import styles;
    • Replaced deprecated authentication and client initialization patterns.
  • YAML parsing:
    • Replaced deprecated yaml.safeLoad and yaml.safeDump with their respectives yaml.loadand yaml.dump.
  • Logging:
    • Introduced LOGGER_LOG_TO_CONSOLE environment variable to control log output to the console;
    • Introduced the ENABLE_REST_VALIDATION_ERRORS environment variable to control REST input validation verbosity. When enabled, ajv will report all validation errors (allErrors: true), which is useful for debugging but may trigger deep object traversal warnings. More info
    • Improved debug and error logs for critical operations.

Let us know your thoughts! ^.^

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
…es in the latest mkdip version

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
- Added support for the /splash endpoint
- Updated optional route parameters to Express v5 syntax ({/:param})
- Adjusted root path and wildcards for routes like /definitions and /harvest

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
- Removed defaultHeaders due to incompatibility with new version.
  See: jdalrymple/gitbeaker#3634

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
…tive AppInsights SDK

- Adds support for a new env variable  to control
  console logging output.
- The third-party  transport has been removed
in favor of a direct integration using the  SDK.

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
- The Redis client accepts  as a valid runtime configuration, but the
  TypeScript definitions expect a different structure. Added @ts-ignore directive
  to bypass the type check.

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
@elaine-mattos elaine-mattos marked this pull request as ready for review July 16, 2025 11:15
@JamieMagee
Copy link
Contributor

Related: clearlydefined/operations#133

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
@henrysachs
Copy link

@JamieMagee @qtomlinson @jeffmendoza @elrayle can someone of you approve the workflows? :)

@JamieMagee
Copy link
Contributor

@henrysachs approved

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
@JamieMagee JamieMagee requested a review from Copilot July 21, 2025 18:53
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR modernizes the ClearlyDefined service by upgrading Node.js from v18 to v20 and updating numerous dependencies to their latest versions. The key changes address deprecated APIs and breaking changes introduced by major version updates of Express.js, GitHub/GitLab API clients, testing libraries, and various other packages.

Key changes include:

  • Node.js upgrade to v20 with dependency updates across the board
  • Express.js v5 migration with route syntax and initialization changes
  • Updated GitHub and GitLab API clients with new authentication patterns

Reviewed Changes

Copilot reviewed 48 out of 54 changed files in this pull request and generated 8 comments.

Show a summary per file
File Description
package.json Updated Node.js version constraint and dependency versions
test/**/*.js Updated test assertions to use deep.equalInAnyOrder instead of deprecated equalInAnyOrder
providers/curation/github.js Updated GitHub API calls to use new rest.* pattern
routes/*.js Updated Express route patterns to v5 syntax
schemas/validator.js Updated AJV validation setup for v8 compatibility
app.js Removed express-init and updated initialization patterns
Comments suppressed due to low confidence (2)

test/routes/harvest.js:28

  • The error message has changed from 'should NOT have' to 'must NOT have', which may indicate a breaking change in the validation library. Verify this is the expected message format from the updated AJV version.
    expect(response._getData()).to.be.eq('data/0 must NOT have additional properties')

test/routes/harvest.js:37

  • Similar to above, the error message format has changed from 'should be' to 'must be'. Ensure this aligns with the updated AJV validation error messages.
    expect(response._getData()).to.be.eq('data/0/tool must be string')

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
- Replace deprecated gitdata.getTags and gitdata.getTag with the new rest.repos.listTags and the rest.git.getTag
- Replace console with structured logging
- Handle annotated and lightweight tags

Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Signed-off-by: ElaineDeMattosSilvaB <[email protected]>
Copy link
Collaborator

@qtomlinson qtomlinson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution!

@qtomlinson
Copy link
Collaborator

@pombredanne Your feedback on this PR would be much appreciated. There are some backward compatibility concerns with Redis persistence that I think we should tackle separately, in a follow-up PR by either Elaine or myself.

Copy link
Member

@pombredanne pombredanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elaine-mattos Here are some comments and nit pickings for your consideration. We discussed that in a recent weekly call and the sentiment is that we should merge and do through testing first.

For the future, it would be really great to avoid these kind of big, massive PR that take a long time to review!

For instance you could have done a completely separate PR for each of:

  • logging enhancements
  • string formatting
  • type annotations
  • minor case changes in test names or comments changes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO we should invalidate an start clean, and not try to keep some compat with existing data which what @elaine-mattos has done here

@@ -1,9 +1,12 @@
{
"name": "service",
"version": "2.3.0",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should bump this, likely to 3.0.0?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants