Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Upgrade to opentracing-javascript 0.14 #117

Merged
merged 4 commits into from
Apr 21, 2020
Merged

Upgrade to opentracing-javascript 0.14 #117

merged 4 commits into from
Apr 21, 2020

Conversation

yurishkuro
Copy link
Member

There is a potentially breaking change in 0.14

opentracing/opentracing-javascript#79

Need to see how this will affect 0.10 JS code.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.1%) to 98.498% when pulling c07489d on upgrade-ot-0.14 into 510cebd on master.

@tiffon
Copy link
Member

tiffon commented Apr 24, 2018

Lets upgrade this to 0.14.2... ?

rmfitzpatrick
rmfitzpatrick reviewed Oct 5, 2018
View reviewed changes
test/opentracing_compatibility.js Outdated
@@ -18,7 +18,7 @@
// OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
// THE SOFTWARE.

import apiCompatibilityChecks from 'opentracing/lib/test/api_compatibility.js';
import * as apiCompatibilityChecks from 'opentracing/lib/test/api_compatibility.js';
Copy link

Choose a reason for hiding this comment

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

apiCompatibilityChecks is a default so that makes this alias incorrect unless you use apiCompatibiltiyChecks.default() below: https://github.com/opentracing/opentracing-javascript/blob/master/src/test/api_compatibility.ts#L92

@RohitRox
Copy link

Any update on this?
There are indeed some breaking changes in 0.14.
Weird thing is there is no release around 0.13.0 on github https://github.com/opentracing/opentracing-javascript/releases

It is desirable to update to 0.14 as it seems like it is latest and also got the types for use with typescript.

@yurishkuro
Copy link
Member Author

yurishkuro commented Apr 20, 2020 •
edited
Loading

@RohitRox something was broken in the tests, I do not have time to investigate. I just rebased to latest master. If you have cycles to fix the tests, feel free to grab this branch.

Test fixes
e1ae92e 
Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro
Copy link
Member Author

Actually, I was able to fix it. Will merge soon.

TODO import
87a43a5 
Signed-off-by: Yuri Shkuro <[email protected]>
@RohitRox
Copy link

@RohitRox something was broken in the tests, I do not have time to investigate. I just rebased to latest master. If you have cycles to fix the tests, feel free to grab this branch.

Thanks for getting this into motion. Will be glad to help with anything.

everett980
everett980 suggested changes Apr 21, 2020
View reviewed changes
Copy link
Collaborator

@everett980 everett980 left a comment

Choose a reason for hiding this comment

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

package-lock.json needs to have the correct url. otherwise LGTM. to be safe, I'd rm -rf node_modules && yarn/npm install && yarn/npm run build && yarn/npm run test to be certain that the lock manual change is good.

package-lock.json Outdated
"resolved": "https://registry.npmjs.org/opentracing/-/opentracing-0.13.0.tgz",
"integrity": "sha1-ajQUQvCdfYZrwR7QPeHjgo49aqs="
"version": "0.14.4",
"resolved": "https://unpm.uberinternal.com/opentracing/-/opentracing-0.14.4.tgz",
Copy link
Collaborator

Choose a reason for hiding this comment

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

this registry needs to be manually changed back to registry.npmjs.org

Regenerate lock file
cebec00 
Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro yurishkuro merged commit 5f0880e into master Apr 21, 2020
@yurishkuro yurishkuro deleted the upgrade-ot-0.14 branch April 21, 2020 17:57
@jaegertracing jaegertracing deleted a comment from codecov bot Apr 21, 2020
Iuriy-Budnikov pushed a commit to agile-pm/jaeger-client-node that referenced this pull request Sep 25, 2021
@OlivierAlbertini @mayurkale22
chore: add commitlint and husky (jaegertracing#128)
44214ac 
Change CONTRIBUTING.md
Resolves jaegertracing#117

Signed-off-by: Olivier Albertini <[email protected]>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rmfitzpatrick rmfitzpatrick rmfitzpatrick left review comments

@everett980 everett980 everett980 requested changes

@black-adder black-adder black-adder approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@yurishkuro @coveralls @tiffon @RohitRox @black-adder @everett980 @rmfitzpatrick