Skip to content

Conversation

@jeysal
Copy link
Contributor

@jeysal jeysal commented Apr 3, 2023

Arch Linux is on OpenSSL 3 (instead of 1.1), like Ubuntu 22.04 (not like 20.04), so currently the mongod startup fails because of missing libcrypto.so.1.1 (unless one has the openssl-1.1 package installed for this or some other reason).

Tested by making this same change locally in the distributed node_modules/mongodb-memory-server-core.

@jeysal jeysal requested review from AJRdev, hasezoey and nodkz as code owners April 3, 2023 09:01
@hasezoey hasezoey added enhancement Module: Version-String Generation This problem is because of the Version-String Generation (MongoBinaryDownloadUrl) labels Apr 3, 2023
@hasezoey
Copy link
Member

hasezoey commented Apr 3, 2023

Looks good to me, could you add tests for this and check if the documentation regarding arch are correct? or should i do it?

@jeysal
Copy link
Contributor Author

jeysal commented Apr 3, 2023

I added a test case similar to those already present, but I don't know how correct e.g. the ubuntu1604 string is since the test cases test some internal functions that are pretty far from the user-facing things that happen. Feel free to adapt.
I didn't see relevant documentation that mentions anything Arch-specific, but could add a changelog entry to mention that this is now working for the current Arch Linux with OpenSSL 3 and not old ones with OpenSSL 1.1

Copy link
Member

@hasezoey hasezoey left a comment

Choose a reason for hiding this comment

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

Thanks for adding tests, but the current test is a bit in the wrong place, and does not actually test your change, because ubuntu 22.04 builds only happen since mongodb version 6.0.4, so a test for that would be necessary (and for mongodb versions not build for 2204, it falls back to older ubuntu versions)

i can accept this change now and do the documentation and tests on my own, or you can do it if you want to

@codecov
Copy link

codecov bot commented Apr 3, 2023

Codecov Report

Merging #762 (51cca3e) into master (ebee094) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #762   +/-   ##
=======================================
  Coverage   90.69%   90.69%           
=======================================
  Files          14       14           
  Lines        1913     1913           
  Branches      496      496           
=======================================
  Hits         1735     1735           
  Misses        169      169           
  Partials        9        9           
Impacted Files Coverage Δ
...ory-server-core/src/util/MongoBinaryDownloadUrl.ts 94.38% <ø> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@jeysal
Copy link
Contributor Author

jeysal commented Apr 3, 2023

Feel free to make the changes and ship, otherwise I can can get to it tomorrow, am out and only on my phone atm

@hasezoey
Copy link
Member

hasezoey commented Apr 3, 2023

added commits to add tests (and move tests), update docs and also updated your commit message (because i dont plan on squashing this PR)

are you fine with the changes or does anything need to be changed?

@jeysal
Copy link
Contributor Author

jeysal commented Apr 4, 2023

@hasezoey looks reasonable :) thanks

@hasezoey hasezoey merged commit afbd177 into typegoose:master Apr 4, 2023
@github-actions
Copy link

github-actions bot commented Apr 4, 2023

@github-actions github-actions bot added the released Pull Request released | Issue is fixed label Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement Module: Version-String Generation This problem is because of the Version-String Generation (MongoBinaryDownloadUrl) released Pull Request released | Issue is fixed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants