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

[graphql] Replace [email protected] w/ @sesamecare-oss/[email protected] #6622

Merged

Conversation

ckane
Copy link
Contributor

@ckane ckane commented Apr 6, 2024

Proposed changes

Replace redlock TS module with @sesamecare-oss/redlock which is a rewrite of the algorithm. The older redlock project hasn't released a new release since a 5.0.0 beta in March of 2023.

Related issues

This was discussed in #6285 - it is not yet determined if this will address the reported problems. I am currently doing some testing, but wanted to open the PR as a preliminary reference for tracking the code changes that need to occur

Checklist

  • I consider the submitted work as finished
  • I tested the code for its functionality
  • I wrote test cases for the relevant uses case (coverage and e2e)
  • I added/update the relevant documentation (either on github or on notion)
  • Where necessary I refactored code to improve the overall quality

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

Copy link

codecov bot commented Apr 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 65.12%. Comparing base (17ea383) to head (9184c53).
Report is 1 commits behind head on release/6.5.0.

Additional details and impacted files
@@              Coverage Diff               @@
##           release/6.5.0    #6622   +/-   ##
==============================================
  Coverage          65.12%   65.12%           
==============================================
  Files                626      626           
  Lines              59784    59784           
  Branches            6663     6660    -3     
==============================================
  Hits               38937    38937           
  Misses             20847    20847           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ckane
Copy link
Contributor Author

ckane commented Apr 9, 2024

This does not solve the problems discussed in #6285 but it has been working without regressions for two days on my instances

@ckane ckane force-pushed the new-redlock-impl branch 2 times, most recently from 89132d5 to 072c0e3 Compare April 10, 2024 22:19
@ckane
Copy link
Contributor Author

ckane commented Apr 15, 2024

This does not solve the problems discussed in #6285 but it has been working without regressions for two days on my instances

The problems discussed in #6285 were fixed in #6640, but still recommend this change due to upstream maintenance state of current redlock package

@Jipegien Jipegien added the community use to identify PR from community label Apr 17, 2024
@Jipegien Jipegien added this to the Release 6.2.0 milestone Apr 17, 2024
@Jipegien Jipegien modified the milestones: Release 6.2.0, Release 6.3.0 Apr 29, 2024
@Jipegien Jipegien modified the milestones: Release 6.3.0, Release 6.4.0 Jun 21, 2024
@SamuelHassine SamuelHassine force-pushed the master branch 3 times, most recently from b710111 to c15ed81 Compare June 23, 2024 20:06
@ckane ckane force-pushed the new-redlock-impl branch from 072c0e3 to cb6a52c Compare July 9, 2024 16:23
@ckane
Copy link
Contributor Author

ckane commented Jul 9, 2024

Rebased against current master branch, and also updated the version of redlock to latest 1.3.1

@ckane
Copy link
Contributor Author

ckane commented Jul 9, 2024

Also not sure what I need to do to clear the "lockfile would be updated" error in the api-tests in the drone CI run. My expectation is I did the npm/yarn upgrade incorrectly and there's something else which needs to be updated too. If you point me at what I need to do I'll fix it and update my tree as well.

@richard-julien
Copy link
Member

Really sorry about this one @ckane , will do some test and merge that in couple of weeks.
For yarn locks you can remove the lock file and regenerate it with yarn install, should resolve the conflicts.

@ckane ckane force-pushed the new-redlock-impl branch 2 times, most recently from 40adf7e to 21a6712 Compare July 20, 2024 23:54
@ckane
Copy link
Contributor Author

ckane commented Jul 20, 2024

Ok thanks @richard-julien - I just pushed a missing dependency that hopefully fixes it. Also re-committed my commits as signed, which I forgot to do earlier.

@SamuelHassine SamuelHassine removed this from the Release 6.4.0 milestone Jul 28, 2024
@ckane ckane force-pushed the new-redlock-impl branch 2 times, most recently from 89fde5b to c1a178f Compare August 7, 2024 18:37
@ckane ckane force-pushed the new-redlock-impl branch 2 times, most recently from a6b264b to 64bd2c4 Compare August 19, 2024 12:26
@richard-julien
Copy link
Member

richard-julien commented Aug 19, 2024

Hi @ckane , can you remove "redlock": "5.0.0-beta.2" in package.json and redo a yarn install.
I think its the reason the PR fail.
Thanks

@ckane ckane force-pushed the new-redlock-impl branch from 64bd2c4 to 57a6851 Compare August 19, 2024 19:08
@ckane ckane force-pushed the new-redlock-impl branch 2 times, most recently from b85c533 to 0abd10b Compare August 19, 2024 20:22
@ckane
Copy link
Contributor Author

ckane commented Aug 20, 2024

Hi @ckane , can you remove "redlock": "5.0.0-beta.2" in package.json and redo a yarn install. I think its the reason the PR fail. Thanks

Thanks @richard-julien, I messed up the rebase. Should be good now.

@ckane ckane force-pushed the new-redlock-impl branch from 0abd10b to b98bae7 Compare August 21, 2024 02:20
@labo-flg labo-flg added the technical improvement Technical refactor or improvement is needed label Nov 4, 2024
@ckane ckane force-pushed the new-redlock-impl branch 2 times, most recently from 14a251e to 1561de4 Compare December 5, 2024 20:09
@richard-julien richard-julien changed the base branch from master to release/6.5.0 December 5, 2024 20:46
@richard-julien
Copy link
Member

Hi @ckane , Im really sorry, im back after some inactivity period, and just change the target from master to 6.5.0.
Looks like there is a conflict :( Can you rebase again and then I will merge the PR ... finally :)

@ckane
Copy link
Contributor Author

ckane commented Dec 5, 2024

Hi @ckane , Im really sorry, im back after some inactivity period, and just change the target from master to 6.5.0. Looks like there is a conflict :( Can you rebase again and then I will merge the PR ... finally :)

No worries - should be rebased against release/6.5.0 now

@ckane
Copy link
Contributor Author

ckane commented Dec 5, 2024

Hm looks like it pulled in a whole bunch of other commits or something. I will try branching a new branch from release/6.5.0 and manually cherry-pick the relevant commits to that

@richard-julien richard-julien merged commit 2c19aac into OpenCTI-Platform:release/6.5.0 Dec 5, 2024
4 checks passed
@richard-julien
Copy link
Member

Merged for 6.5.0. Thanks @ckane

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community use to identify PR from community technical improvement Technical refactor or improvement is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants