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

Thchang/2051 protobufjs version bump #2361

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

TienHao
Copy link
Contributor

@TienHao TienHao commented Mar 21, 2024

Description

this PR closes #2051

The package protobufjs was updated to the latest 7.2.6, since there was a bug when we implemented 6.10.8 and it did not be fixed until 7.0.0. And I also added package protobufjs-cli for the building tools(pbjs, pbts) have been moved from protobufjs after 7.0.0. Therefore, a update for build_proto.sh in the carta-protobuf was also needed.

The companion carta-protobuf PR: CARTAvis/carta-protobuf#95

Checklist

For linked issues (if there are):

  • assignee and label added
  • ZenHub issue connection, board status, and estimate updated

For the pull request:

  • reviewers and assignee added
  • ZenHub estimate, milestone, and release (if needed) added
  • changelog updated / no changelog update needed
  • unit test added (for functions with no dependenies)
  • API documentation added (for public variables and methods in stores)

For dependencies:

  • e2e test passing / corresponding fix added / new e2e test created
  • protobuf version bumped / no protobuf version bumped needed
  • protobuf updated to the latest dev commit / no protobuf update needed
  • corresponding ICD test fix added (BackendService changed) / no ICD test fix needed (BackendService unchanged)
  • user manual prepared (for large new features)

 * added package protobufjs-cli for protobuf building

 * fixed import of package long
@TienHao TienHao added the awaiting code review For pull requests that require code review label Mar 21, 2024
@TienHao TienHao requested a review from YuHsuan-Hwang March 21, 2024 05:57
@YuHsuan-Hwang
Copy link
Collaborator

@TienHao Please also send a protobuf PR and add the link in the description, thanks.

Copy link
Collaborator

@YuHsuan-Hwang YuHsuan-Hwang left a comment

Choose a reason for hiding this comment

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

Looks good.

Copy link
Collaborator

@loveluthien loveluthien left a comment

Choose a reason for hiding this comment

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

Ubuntu is also fine.

@TienHao TienHao marked this pull request as ready for review March 27, 2024 08:25
@acdo2002
Copy link
Contributor

acdo2002 commented Apr 4, 2024

Could you add ICD-RxJS protobuf version bump as another dependent check box?
Although there is no BackendService.ts change, the ICD-RxJS also used protobufjs version, so ICD-RxJS needs the companion PR. I have pushed the PR for it in here

Copy link
Collaborator

@kswang1029 kswang1029 left a comment

Choose a reason for hiding this comment

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

Tested with the corresponding backend PR CARTAvis/carta-backend#1367 and the carta-protobuf PR CARTAvis/carta-protobuf#95. All e2e tests are passing. 👍

@YuHsuan-Hwang YuHsuan-Hwang added awaiting merge For pull requests ready for merge or pending backend/protobuf changes and removed awaiting code review For pull requests that require code review labels Apr 23, 2024
@YuHsuan-Hwang YuHsuan-Hwang removed the awaiting merge For pull requests ready for merge or pending backend/protobuf changes label Apr 23, 2024
@YuHsuan-Hwang YuHsuan-Hwang merged commit b6f00cc into dev Apr 23, 2024
6 checks passed
@YuHsuan-Hwang YuHsuan-Hwang deleted the thchang/2051_protobufjs_version_bump branch April 23, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: PR test and review
Development

Successfully merging this pull request may close these issues.

Bump protobufjs from 6.9.0 to 6.10.3
6 participants