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.

Sign up for GitHub

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

Jump to bottom

update re2 for linux, darwin, and windows #1453

Merged
merged 1 commit into from
Apr 14, 2022
Merged

update re2 for linux, darwin, and windows #1453

merged 1 commit into from
Apr 14, 2022

Conversation

ananzh
Copy link
Member

@ananzh ananzh commented Apr 13, 2022 •
edited by kavilla
Loading

Description

Update re2 to 83

Screen Shot 2022-04-13 at 11 53 52

Issues Resolved

#1452

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@ananzh
update re2 fix node imcompatible issue
d1f3e39 
Issue Resolved:
opensearch-project#1452

Signed-off-by: Anan Zhuang <ananzh@amazon.com>
@ananzh ananzh requested a review from a team as a code owner April 13, 2022 19:17
@ananzh ananzh self-assigned this Apr 13, 2022
@ananzh ananzh added build Build related additions or modifications v2.0.0 labels Apr 13, 2022
tmarkley
tmarkley reviewed Apr 13, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley 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 to me, but can we add a test (functional test in this case) to cover this specific case that we're looking for?

kavilla
kavilla reviewed Apr 14, 2022
View reviewed changes
Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

Does this one address the node combability or is it more so bumping to the latest and greatest? If so we should update title of this PR.

@ananzh
Copy link
Member Author

ananzh commented Apr 14, 2022

Does this one address the node combability or is it more so bumping to the latest and greatest? If so we should update title of this PR.

This is a bump. But these three also have the incompatibility issue. If run OSD via yarn start, then no issue. If build OSD to an artifact, then unzip and run ./bin/opensearch-dashboards, we would see similar version incompatibility issue. I will modify the title a bit to make it more clearer.

@ananzh
Copy link
Member Author

ananzh commented Apr 14, 2022 •
edited
Loading

Looks good to me, but can we add a test (functional test in this case) to cover this specific case that we're looking for?

Will add a unit test. If run OSD via yarn start, then no issue. Issue only happens when run OSD with build artifact. Infra has some tests on this. I will add a unit test for the function I made changes. It will cover two things:
1)PatchNativeModules could download the correct package
2)PatchNativeModules could differentiate .tar.gz and .gz and unzip the package correctly

I decide to add it here because this is the PR I made changes to the PatchNativeModules function.

@ananzh ananzh changed the title update re2 fix node imcompatible issue update re2 for linux, darwin, and windows Apr 14, 2022
tmarkley
tmarkley approved these changes Apr 14, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley 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! I think we can track the work needed to have BWC tests build an artifact and test for regressions.

@ananzh ananzh merged commit 1492bfc into opensearch-project:main Apr 14, 2022
@ananzh ananzh deleted the re2-update branch June 1, 2022 21:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmarkley tmarkley tmarkley approved these changes

@kavilla kavilla kavilla approved these changes

@boktorbb boktorbb Awaiting requested review from boktorbb

@AMoo-Miki AMoo-Miki Awaiting requested review from AMoo-Miki AMoo-Miki is a code owner

Assignees

@ananzh ananzh

Labels
build Build related additions or modifications v2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ananzh @tmarkley @kavilla