-
Notifications
You must be signed in to change notification settings - Fork 407
feat(openfeature): vendor the flagging provider instead of an optional peer dependency #9570
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
Changes from all commits
4a2d6be
b1fe7a9
82eacf2
b041ce0
6effd2d
433f9dd
f73a273
0baab9a
a64371e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
This file was deleted.
This file was deleted.
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| 'use strict' | ||
|
|
||
| // Static fallback for file tracers that do not recognize the optional-peer wrapper. | ||
| require('@datadog/openfeature-node-server') | ||
| // Static fallback for file tracers that do not recognize the vendored provider's lazy require. | ||
| require('./vendor/dist/@datadog/openfeature-node-server') | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -184,7 +184,6 @@ | |
| "@datadog/native-appsec": "11.0.1", | ||
| "@datadog/native-iast-taint-tracking": "4.2.0", | ||
| "@datadog/native-metrics": "3.1.2", | ||
| "@datadog/openfeature-node-server": "2.0.2", | ||
| "@datadog/pprof": "5.18.0", | ||
| "@datadog/wasm-js-rewriter": "5.0.1", | ||
| "@opentelemetry/api": ">=1.0.0 <1.10.0", | ||
|
|
@@ -197,6 +196,7 @@ | |
| "@babel/helpers": "^8.0.0", | ||
| "@eslint/eslintrc": "^3.3.5", | ||
| "@eslint/js": "^10.0.1", | ||
| "@datadog/openfeature-node-server": "2.1.0", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. exact version pin needed?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We started doing this in #5995. Unfortunately the PR doesn't explain the reasoning, but I prefer to keep it the same so that it stays consistent. The good news is that it doesn't really change anything because the vendoring is done from the lockfile, so pin or no pin is effectively the same there, and it makes sense to keep the dev dependency in line with that. This is also automatically updated weekly by Dependabot. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Agreed that with vendoring there's no difference. It might be worth documenting why vendoring is necessary though (or pointing us to existing documentation). Getting to a place where transient packages can be upgraded by customers without a version bump of
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It also does the opposite in that it allows zero-day exploits to actually land to begin with. So vendoring with a 5 days lag allows us to never ship the exploit to begin with. If the exploit is discovered more than 5 days later, then we can expedite a fix as well without waiting for Dependabot. Trade-offs all around, but given the other benefits of vendoring and the fact that we already do this for everything else (except native extensions because it's not possible to vendor them and we instead avoid installing entirely where not supported) I'd rather keep things consistent. Happy to revisit any past decisions later on but that's out of scope of this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's what I was calling out by referencing zero day attacks. It's a good reason to keep vendoring. Like you said, tradeoffs either way. It's just unclear on my end if that was motivation behind vendoring. In any case, you should be good to go with this change. |
||
| "@msgpack/msgpack": "^3.1.3", | ||
| "@openfeature/core": "^1.11.0", | ||
| "@openfeature/server-sdk": "~1.22.0", | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.