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

[ioredis plugin] fix: change supportedVersions to >1 <5 #671

Merged

Conversation

naseemkullah
Copy link
Member

@naseemkullah naseemkullah commented Jan 7, 2020

Which problem is this PR solving?

  • ioredis v4.x.x should be patched as it is current version of ioredis, furthermore redis 2.x.x throws TypeError: Redis is not a constructor ioredis versions 2.x.x, 3.x.x and 4.x.x should be patched

Short description of the changes

  • changes supported version from ^2.x.x to ^4.x.x

@codecov-io
Copy link

codecov-io commented Jan 7, 2020

Codecov Report

Merging #671 into master will decrease coverage by 1.72%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #671      +/-   ##
==========================================
- Coverage   92.75%   91.03%   -1.73%     
==========================================
  Files         228      225       -3     
  Lines       10462    10414      -48     
  Branches      935      959      +24     
==========================================
- Hits         9704     9480     -224     
- Misses        758      934     +176
Impacted Files Coverage Δ
packages/opentelemetry-core/test/utils/url.test.ts 50% <0%> (-50%) ⬇️
...pentelemetry-core/test/internal/validators.test.ts 50% <0%> (-50%) ⬇️
...elemetry-core/test/trace/spancontext-utils.test.ts 55.55% <0%> (-44.45%) ⬇️
...lemetry-core/test/trace/ProbabilitySampler.test.ts 56.52% <0%> (-43.48%) ⬇️
...s/opentelemetry-core/test/trace/NoopTracer.test.ts 60% <0%> (-40%) ⬇️
...s/opentelemetry-core/test/context/B3Format.test.ts 63.39% <0%> (-36.61%) ⬇️
...ges/opentelemetry-core/test/trace/NoopSpan.test.ts 63.63% <0%> (-36.37%) ⬇️
...s/opentelemetry-core/test/trace/tracestate.test.ts 65.06% <0%> (-34.94%) ⬇️
...ntelemetry-core/test/trace/NoRecordingSpan.test.ts 71.42% <0%> (-28.58%) ⬇️
...ackages/opentelemetry-core/src/platform/node/id.ts 71.42% <0%> (-28.58%) ⬇️
... and 132 more

@dyladan
Copy link
Member

dyladan commented Jan 7, 2020

Does this plugin work with 3.x.x? Can we do ["^3.0.0", "^4.0.0"]?

@vmarchaud
Copy link
Member

I believe the code in its current state support both 2,3,4 so it should be >=2 <5

@dyladan
Copy link
Member

dyladan commented Jan 7, 2020

@vmarchaud according to @naseemkullah he had trouble using it with 2

@naseemkullah
Copy link
Member Author

naseemkullah commented Jan 7, 2020

I'd be happy to test all versions with a local plugin that has an updated supportedVersions but don't know how, can someone please explain?

I have the code to test it (as per ioredis example PR), just need to know how to point to local plugin

@dyladan
Copy link
Member

dyladan commented Jan 7, 2020

@naseemkullah what I would do is change the example package.json to point to the plugin by file path instead of installing it by version number. Then when you compile your local working version, the example will use your local version with whatever changes you've made. LMK if that doesn't make sense.

@vmarchaud
Copy link
Member

@naseemkullah Another solution is running the test with the version you want to test, you only need to yarn add ioredis@2 && yarn test to test against the version 2. And you repeat for each version you want to test.
Afterwards you just reset the changes made to the package.json and the lock and you are good

@obecny
Copy link
Member

obecny commented Jan 7, 2020

if you use lerna to bootstrap your packages then it will be possible to debug and use the local version for you examples, just enable the examples in lerna.json and run npm run bootstrap . TBH I still don't fully agree with removing the examples from lerna bootstrap as examples are really an easy way for debugging stuff (for web packages you will find npm run watch to be able to debug certain package easily)

@dyladan
Copy link
Member

dyladan commented Jan 7, 2020

@obecny this was done because the symlink was causing the node plugin loader to be confused about where node_modules was. afaik you can bootstrap all packages except @opentelemetry/node

@naseemkullah
Copy link
Member Author

Thanks for the advice everyone, I was able to properly test locally with ioredis versions 2.x.x, 3.x.x and 4.x.x which all yield:

const redis = new Redis();
              ^

TypeError: Redis is not a constructor

I'll continue to investigate.

@dyladan
Copy link
Member

dyladan commented Jan 8, 2020

Sounds like an issue with the esModuleInterop tsconfig compiler option to me

@dyladan
Copy link
Member

dyladan commented Jan 20, 2020

@naseemkullah what is the status of this?

@naseemkullah
Copy link
Member Author

I cannot get passed the above mentioned error and require support

@dyladan
Copy link
Member

dyladan commented Jan 21, 2020

@naseemkullah I looked into this and it is actually the plugin causing the ioredis module to break. Issue #713 with open PR #714

@naseemkullah
Copy link
Member Author

Excellent!

@dyladan dyladan changed the title [ioredis plugin] fix: change supportedVersions to 4.x.x [ioredis plugin] fix: change supportedVersions to >1 <5 Jan 22, 2020
@dyladan dyladan added bug Something isn't working size/XS labels Jan 22, 2020
@dyladan dyladan added the Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.) label Jan 23, 2020
@dyladan
Copy link
Member

dyladan commented Jan 24, 2020

@mayurkale22 if this looks good to you this is ok to merge

@@ -23,7 +23,7 @@ import { VERSION } from './version';
export class IORedisPlugin extends BasePlugin<typeof ioredisTypes> {
static readonly COMPONENT = 'ioredis';
static readonly DB_TYPE = 'redis';
readonly supportedVersions = ['^2.0.0'];
readonly supportedVersions = ['>1 <5'];
Copy link
Member

Choose a reason for hiding this comment

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

Do we support version 1.x.x? This version string will match that I believe? I think you're looking for '>=2 <5'

Copy link
Member Author

Choose a reason for hiding this comment

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

According to https://semver.npmjs.com/ it's fine 🤷‍♂

@dyladan dyladan merged commit 7949e3f into open-telemetry:master Jan 28, 2020
@naseemkullah naseemkullah deleted the ioredis-supported-versions branch April 9, 2021 18:18
pichlermarc pushed a commit to dynatrace-oss-contrib/opentelemetry-js that referenced this pull request Dec 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Merge:LGTM This PR is ready to be merged by a Maintainer (has enough valid approvals, successful build, etc.)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants