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

Upgrade ubuntu to 22.04 second try #71

Merged
merged 4 commits into from
Sep 26, 2022
Merged

Conversation

tlbdk
Copy link
Contributor

@tlbdk tlbdk commented Sep 20, 2022

Move Ubuntu 18.04 source setup to common image
Install libssl1.1 so we support common tools build for older ubuntu versions
Workaround issue with newer libssl and older node versions
Also tag images with node version and commit sha

@tlbdk tlbdk marked this pull request as draft September 20, 2022 19:55
@tlbdk
Copy link
Contributor Author

tlbdk commented Sep 20, 2022

@tlbdk
Copy link
Contributor Author

tlbdk commented Sep 20, 2022

Install libssl1.1 so we support common tools build for older ubuntu versions
Workaround issue with newer libssl and older node versions
@tlbdk tlbdk marked this pull request as ready for review September 20, 2022 20:52
@tlbdk tlbdk requested a review from a team September 20, 2022 20:52
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@chribsen
Copy link
Contributor

Is the crypto issue with nodejs also fixed in this PR?

@mathiastj
Copy link
Contributor

I think we need to bump node to 16.17 to fix the crypto issue: nodejs/node#42919

README.md Show resolved Hide resolved
@tlbdk
Copy link
Contributor Author

tlbdk commented Sep 21, 2022

nodejs/node#42919

I just tested the example script on v16.16.0 and it works:

root@b436cdf917fb:/# node test.js 
ArrayBuffer {
  [Uint8Contents]: <fe fe 48 6a c6 0b ef b4 ba ab 79 03 bf 59 e3 7a 04 ab 69 c6 9d cb 62 6c de fe bc b9 f0 04 28 1c 7c 1f 12 41 e7 47 fb 38 ce ed 87 97 f7 01 a1 4a ad 18 d6 57 f5 c1 5b f0 1b 4a 41 92 78 b9 fd 54 7a 26 a9 a6 7f d0 2d df e8 18 ed a6 13 a7 9d e1 c0 29 17 eb ec d0 fc 58 92 53 d5 55 66 3f 16 e9 f5 cb 04 fd ... 12245 more bytes>,
  byteLength: 12345
}
done.
root@b436cdf917fb:/# node --version
v16.16.0

I would prefer to do the node upgrade in a separate pull.

@tlbdk
Copy link
Contributor Author

tlbdk commented Sep 21, 2022

Is the crypto issue with nodejs also fixed in this PR?

It was another crypto issue you had, and it was because node 12.x was used in the image, node 12.x is out of support. I added the fix for it anyway to the image.

@chribsen chribsen self-requested a review September 22, 2022 09:09
@chribsen chribsen self-assigned this Sep 23, 2022
Copy link
Contributor

@chribsen chribsen 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, but can you wait with merging until Monday?

@tlbdk
Copy link
Contributor Author

tlbdk commented Sep 23, 2022

Looks good, but can you wait with merging until Monday?

yes, will do

@tlbdk tlbdk merged commit 87e9047 into master Sep 26, 2022
@delete-merged-branch delete-merged-branch bot deleted the revert-70-cd/revert-latest-changes branch September 26, 2022 09:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants