Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
10597 fix package.json linting errors #13070
10597 fix package.json linting errors #13070
Changes from 36 commits
bd3df53deead72972104aba94b58138ca62d72bf0d5fab6808d4e840161cb3342959a102e55c0ee027708960d23ffcc2f95f2d14776d46e04d3b9e114de05b97b6d926e432e41803697ea33dfdb79dafe9e33ca881bf0bfb912d5bf5005696a2db022a13174b55e8716f09ffce3f8ae706a1cb25c61e0997f6b78c23bbe528984d10823b12File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Calling out this change - wasn't sure the impact so I'd like to have someone 👍 or 👎 this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason this was set to 8.3.0 and not 8.0.0? Feel free to let me know if I should suppress the linter rule that enforces this here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe so, unless @hectorhdzg knows of something that the exporter needs that requires 8.3.x?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenTelemetry doesn't support node < 8.5.x actually we need to update this to >=8.5.0
https://github.com/open-telemetry/opentelemetry-js#node-support
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change that to 8.5.0 and turn off the linter error that complains (or allow overriding the node version in that rule). I can also update the README if you'd like since I'll be there: https://github.com/Azure/azure-sdk-for-js/tree/master/sdk/monitor/opentelemetry-exporter-azure-monitor#prerequisites
Does that work for you @hectorhdzg ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/cc @bterlson @ramya-rao-a can we update our min-bar to Node 8.5.0 across the board?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Originally I wanted to turn that rule off for
opentelemetry-exporter-azure-monitor; however, that rule couples the required node version with the presence of that key inenginesso turning it off means there's nothing to stop someone from removing theenginesfield entirely.So instead I'm providing an option called
overrideNodeVersionto @azure/azure-sdk/ts-package-json-engine-is-presentYou can see the change in this commit: 28984d1
I also cleaned up the docs (they weren't accurate), added info about that override flag, and updated
opentelemetry-exporter-azure-monitorto specify 8.5.0 as the min node version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@maorleger this is great!
@hectorhdzg I wonder if
nodev8.5.0 is the minimum version supported, why not use this version for the dev dependency instead of v10?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@deyaaeldeen do you mean @types/node package?, that is interesting I can see OpenTelemetry have dependencies on latest version of the package.
https://github.com/open-telemetry/opentelemetry-js/blob/master/packages/opentelemetry-api/package.json#L58
So the issue is related to the code using classes that doesn't exist in previous node.js versions and potentially causing issues when running in older versions, makes total sense to update it in my opinion, I can take care of this update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@xirzec if you're asking from a policy perspective, I think yes. I'd be fine with 8.17 being a min bar even, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hectorhdzg yeah I think the node version(s) specified in
enginesshould match that of@types/node. If using the latest node version is necessary, then having that as the version in engines and@types/nodeshould be fine. @xirzec @bterlson correct me if I am wrong about this.P.S. If you were to change the version of
@types/node, you also need to change the version incommon/config/rush/common-versions.json.