Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed workspaces issues #1266

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Fixed workspaces issues #1266

wants to merge 7 commits into from

Conversation

kirrg001
Copy link
Contributor

@kirrg001 kirrg001 commented Aug 9, 2024

An alternative approach for https://jsw.ibm.com/browse/INSTA-7722 is to workaround the problems:

Possible ideas:

  • try a different installation strategy to workaround the missing feature nohoist ([RRFC] Add nohoist option for workspaces npm/rfcs#287) -> problem: peer dependency requirements do not work such as "typeorm requires mssql v10, but we already test against v11 on root".
    • what if we put all deps to their workspace? problem consists, typeorm and mssql would land on the same workspace
  • add a script which protects running a wrong library version in tests e.g. you have pino v8 and v9 installed and you run the pino test against both major versions, we need to ensure npm is loading the correct dependency
  • migrate to .tav (test all versions) https://www.npmjs.com/package/test-all-versions
  • remove all dev deps, extend currencies.json with version support, make it available when running a test, iterate over the versions and a script will auto install this dep.
    • disadvantage: no npm caching, slow c++ installs (how to deal with that)

Problems we have to fix:

  • npm loads wrong dependency, because a dependency on the workspace (!) has the same sub dependency and npm loads this instead of the root dependency. We had that with Tedious.
  • Typeorm has a peer dependency to mssql v10, but we already test against mssql v11 on root - we are getting in trouble how to install the dependencies

@kirrg001 kirrg001 force-pushed the fix-workspaces-issues branch from 9b8db89 to 8a2fd7b Compare September 2, 2024 09:11
@@ -122,6 +122,7 @@
{
"name": "@elastic/elasticsearch",
"policy": "45-days",
"versions": ["7.9.0", "7.17.0", "8.15.0"],
Copy link
Contributor Author

@kirrg001 kirrg001 Sep 4, 2024

Choose a reason for hiding this comment

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

"versions": ">= 7"
"versions": ">= 7, 8.15.0"

testUtils.getVersions(versions, nameOfTheLib)
[latest7, latest8, latest9]

What is the currency bot doing?

  • Major updates?
  • Visibility: we want to know if a minor versions comes out and we are able to check the features
  • Visibility: on what is tested

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"versions": ">= 7"

If we use this notation in the currencies.json, we need to initially generate the currencies.json the following:

{
   "name": "@elastic/elasticsearch",
   "supportedVersions": ">= 7"
   "testedVersions": ["7.9.0", "8.15.0"]
   ...

In the test we use:

CURRENCY.testedVersions.forEach(...)

The currency bot in the night still runs a major and minor update check. If there is a new version, a PR is created for the currencies.json diff.

e.g. for major:

{
   "name": "@elastic/elasticsearch",
   "supportedVersions": ">= 7"
   "testedVersions": ["7.9.0", "8.15.0", "9.0.0"]
   ...

e.g. for minor

{
   "name": "@elastic/elasticsearch",
   "supportedVersions": ">= 7"
   "testedVersions": ["7.9.0", "8.16.0"]
   ...

This gives us visibility.
As soon as we merge the PR for the currencies.json, the testedVersions array is updated and the tests run against v9 or 8.16.0 or whatever.

"supportedVersions": ">= 7"

This can be publicly displayed and is the base how to generate the testedVersions array.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect, this is a great approach!

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

Successfully merging this pull request may close these issues.

2 participants