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

Node 18 #6752

Merged
merged 6 commits into from
Feb 22, 2024
Merged

Node 18 #6752

merged 6 commits into from
Feb 22, 2024

Conversation

AndrewChubatiuk
Copy link
Contributor

@AndrewChubatiuk AndrewChubatiuk commented Feb 7, 2024

What type of PR is this?

Node 18 is not the latest stable, but at least still maintained in contrast to 16 and 14. Tested it with Node 20 as well, but currently there's no cypress image with node 20

  • Refactor
  • Feature
  • Bug Fix
  • New Query Runner (Data Source)
  • New Alert Destination
  • Other

Description

How is this tested?

  • Unit tests (pytest, jest)
  • E2E Tests (Cypress)
  • Manually
  • N/A

Related Tickets & Documents

Mobile & Desktop Screenshots/Recordings (if there are UI changes)

Copy link

codecov bot commented Feb 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.42%. Comparing base (52cd6ff) to head (568407c).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #6752   +/-   ##
=======================================
  Coverage   63.42%   63.42%           
=======================================
  Files         162      162           
  Lines       13173    13173           
  Branches     1819     1819           
=======================================
  Hits         8355     8355           
  Misses       4522     4522           
  Partials      296      296           

@AndrewChubatiuk AndrewChubatiuk force-pushed the node-18 branch 3 times, most recently from 4323826 to 58e2e5d Compare February 10, 2024 06:25
@AndrewChubatiuk
Copy link
Contributor Author

@justinclift @guidopetri could you please review it?

@AndrewChubatiuk
Copy link
Contributor Author

@justinclift @guidopetri are you still maintaining a repo?

@justinclift
Copy link
Member

@AndrewChubatiuk I'm taking some time off after hitting the early stages of burn out. 😦

"analyze": "yarn clean && BUNDLE_ANALYZER=on webpack",
"analyze:build": "yarn clean && NODE_ENV=production BUNDLE_ANALYZER=on webpack",
"analyze": "yarn clean && BUNDLE_ANALYZER=on NODE_OPTIONS=--openssl-legacy-provider webpack",
"analyze:build": "yarn clean && NODE_ENV=production BUNDLE_ANALYZER=on NODE_OPTIONS=--openssl-legacy-provider webpack",
Copy link
Member

Choose a reason for hiding this comment

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

Sounds like this is needed for NodeJS 18 to work with the Redash repo then yeah?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is needed to work with machines with old version of openssl

Copy link
Member

Choose a reason for hiding this comment

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

Cool, no worries. 😄

Copy link
Member

@justinclift justinclift left a comment

Choose a reason for hiding this comment

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

This seems like it should be ok. 😄

@justinclift justinclift enabled auto-merge (squash) February 22, 2024 21:00
auto-merge was automatically disabled February 22, 2024 21:38

Head branch was pushed to by a user without write access

Copy link
Member

@justinclift justinclift 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. 😄

@justinclift justinclift enabled auto-merge (squash) February 22, 2024 21:50
@justinclift justinclift merged commit 094984f into getredash:master Feb 22, 2024
14 of 15 checks passed
@justinclift
Copy link
Member

Thanks for getting this done @AndrewChubatiuk. I'm going to be less active on GitHub for maybe a week or so, but feel free to ping me if something needs attention. 😄

@AndrewChubatiuk
Copy link
Contributor Author

AndrewChubatiuk commented Feb 22, 2024

@justinclift wanted to merge #6674. MR is failing, but this is because it's running scenarios from master

@AndrewChubatiuk AndrewChubatiuk deleted the node-18 branch February 22, 2024 23:02
@justinclift
Copy link
Member

@AndrewChubatiuk Interesting. That looks like it'll need some proper thinking about to fix, which I don't have the mental bandwidth for at the moment. 😦

Saying that because I'm under the impression it shouldn't be running scenarios from master when the PR is in a different branch.

If that's what's really happening, then that sounds like a problem in itself. 😦

@eradman
Copy link
Collaborator

eradman commented Feb 27, 2024

@AndrewChubatiuk can you update https://github.com/getredash/redash/wiki/Local-development-setup to match? I assume this change implies that we should be running nvm use 18 for development now.

@snickerjp
Copy link
Member

I have made an UPDATE to the wiki.
https://github.com/getredash/redash/wiki/Local-development-setup/_compare/a534424f2d8da9221e29019ed12bef94c6d4ed2f...c5c70019e6e0ae773b9a35f6d5a722ccde710616

@justinclift
Copy link
Member

Awesome. Thanks heaps @snickerjp. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants