fix: live tunnel arch issue #4064

lukasholzer · 2022-01-17T15:52:30Z

🎉 Thanks for submitting a pull request! 🎉

Summary

Fixes #3561

This will now stop working for debian arm64 so we need to build the image first before merging.

The matching PR for cross compiling the different CPU architectures https://github.com/netlify/live-tunnel/pull/112

For us to review and ship your PR efficiently, please perform the following steps:

Open a bug/issue before writing your code 🧑‍💻. This ensures we can discuss the changes and get feedback from everyone that should be involved. If you`re fixing a typo or something that`s on fire 🔥 (e.g. incident related), you can skip this step.
Read the contribution guidelines 📖. This ensures your code follows our style guide and
passes our tests.
Update or add tests (if any source code was changed or added) 🧪
Update or add documentation (if features were changed or added) 📝
Make sure the status checks below are successful ✅

A picture of a cute animal (not mandatory, but encouraged)

github-actions · 2022-01-17T15:53:33Z

📊 Benchmark results

Comparing with 7ec8cdc

Package size: 355 MB

⬆️ 0.00% increase vs. 7ec8cdc

^  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB  448 MB         
│   ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐    ┌──┐          
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |          
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |          
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |   355 MB 
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    ┌──┐  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
│   |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |  |    |▒▒|  
└───┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴────┴──┴──>
    T-12    T-11    T-10    T-9     T-8     T-7     T-6     T-5     T-4     T-3     T-2     T-1      T

Legend

T-30 (7ec8cdc): 355 MB
T-29 (1c7eb3b): 355 MB
T-28 (34c00bb): 355 MB
T-27 (c421106): 355 MB
T-26 (2b94f41): 355 MB
T-25 (14ce2eb): 355 MB
T-24 (1fa642e): 355 MB
T-23 (3079e8e): 355 MB
T-22 (898cd5a): 355 MB
T-21 (41494be): 355 MB
T-20 (25ef862): 355 MB
T-19 (1c165e0): 354 MB
T-18 (6e43165): 354 MB
T-17 (72856c7): 354 MB
T-16 (aedd365): 354 MB
T-15 (7f85067): 354 MB
T-14 (662e01b): 448 MB
T-13 (0256222): 448 MB
T-12 (9acf1a9): 448 MB
T-11 (825915e): 448 MB
T-10 (6339919): 448 MB
T-9 (f0b31b9): 448 MB
T-8 (417185b): 448 MB
T-7 (6542ce2): 448 MB
T-6 (be9cf6d): 448 MB
T-5 (8351e25): 448 MB
T-4 (41e6729): 448 MB
T-3 (5e6df42): 448 MB
T-2 (70df1b8): 448 MB
T-1 (064049b): 448 MB
T (current commit): 355 MB

ehmicky · 2022-01-17T16:04:28Z

src/lib/exec-fetcher.js

@@ -1,13 +1,15 @@
 // @ts-check
 const path = require('path')
 const process = require('process')
+const { stringify } = require('querystring')


[sand] What are your thoughts on using new URL() and url.searchParams.set() instead?
This is mostly equivalent, except it uses the DOM API instead of Node.js core API. What are your thoughts on this?

Yea why not I don't have a preference to be honest -> is there any benefit?

If we had more code to share with the front-end, it would be better. And Node.js has been moving in that direction lately of using more and more the DOM API, as opposed to to its own core Node.js modules (URI, AbortSignal, EventTarget, globalThis, ES modules, Blob, ReadableStream/WritableStream/TransformStream, Worker, Web Crypto, performance, structuredClone), which seems a good thing at a higher level, and is also closer to the direction Deno is heading.

However, this is more of a conceptual benefit in this case, since we do not intend to run this file anywhere but on a terminal in this case, so either way could work. 👍

I just realized this too: querystring is marked as "legacy" in the Node.js documentation.

I'll change it for you ;)

bin/run

erezrokah

This looks good to me.
I'll @ehmicky and @lukasholzer resolve https://github.com/netlify/cli/pull/4064/files#r786137881

ehmicky · 2022-01-18T13:50:08Z

https://github.com/netlify/cli/pull/4064/files#r786137881 was more of a side note, please feel free to dismiss! :)

lukasholzer · 2022-01-18T13:52:47Z

@erezrokah who is the best one to review the matching PR in the live tunnel repo? netlify/live-tunnel#112

lukasholzer · 2022-01-18T17:51:57Z

just tried the new binary for arm64 on my m1 mac and it works 🥳

lukasholzer · 2022-01-18T17:55:24Z

@charliegerard I hope it works now on the raspberry pi ;)

I added now following CPU architectures: https://github.com/netlify/live-tunnel-client/releases/tag/v0.2.10

charliegerard · 2022-01-18T22:11:12Z

omg thank you so much for working on it!! 😃 🙌 Right on time for a talk I have to give next week 🥳 I'll test it a bit later today!

lukasholzer added 2 commits January 17, 2022 13:03

fix(dev): use the correct binary for operating system architecture.

806a5c5

fix: fixes an issue with downloading the wrong arch for system one

118dc75

lukasholzer requested review from ehmicky and erezrokah January 17, 2022 15:52

github-actions bot added the type: bug code to address defects in shipped code label Jan 17, 2022

lukasholzer changed the title ~~Fix/live tunnel arch issue~~ fix: live tunnel arch issue Jan 17, 2022

ehmicky reviewed Jan 17, 2022

View reviewed changes

ehmicky previously approved these changes Jan 17, 2022

View reviewed changes

chore: fix linux arch

b702695

lukasholzer dismissed ehmicky’s stale review via b702695 January 18, 2022 09:32

erezrokah reviewed Jan 18, 2022

View reviewed changes

bin/run Outdated Show resolved Hide resolved

chore: pr feedback and handle ia32 architecture

f0e98ef

lukasholzer requested a review from erezrokah January 18, 2022 13:42

Merge branch 'main' into fix/live-tunnel-arch-issue

f604a4a

erezrokah previously approved these changes Jan 18, 2022

View reviewed changes

Merge branch 'main' into fix/live-tunnel-arch-issue

1a712d8

lukasholzer added the automerge Add to Kodiak auto merge queue label Jan 18, 2022

Merge branch 'main' into fix/live-tunnel-arch-issue

32786a2

chore: remove querystring and use URL

937409f

lukasholzer dismissed erezrokah’s stale review via 937409f January 19, 2022 09:29

lukasholzer requested review from erezrokah and ehmicky January 19, 2022 09:53

erezrokah approved these changes Jan 19, 2022

View reviewed changes

Merge branch 'main' into fix/live-tunnel-arch-issue

9bc7595

Merge branch 'main' into fix/live-tunnel-arch-issue

d27ea86

kodiakhq bot merged commit 41c2fdf into main Jan 19, 2022

kodiakhq bot deleted the fix/live-tunnel-arch-issue branch January 19, 2022 12:07

token-generator-app bot mentioned this pull request Jan 19, 2022

chore(main): release 8.9.10 #4085

Merged

token-generator-app bot mentioned this pull request May 26, 2022

chore(main): release 10.3.3 #4629

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: live tunnel arch issue #4064

fix: live tunnel arch issue #4064

lukasholzer commented Jan 17, 2022 •

edited

Loading

github-actions bot commented Jan 17, 2022 •

edited

Loading

ehmicky Jan 17, 2022

lukasholzer Jan 17, 2022

ehmicky Jan 17, 2022 •

edited

Loading

ehmicky Jan 18, 2022

lukasholzer Jan 19, 2022

erezrokah left a comment

ehmicky commented Jan 18, 2022

lukasholzer commented Jan 18, 2022

lukasholzer commented Jan 18, 2022

lukasholzer commented Jan 18, 2022

charliegerard commented Jan 18, 2022

fix: live tunnel arch issue #4064

fix: live tunnel arch issue #4064

Conversation

lukasholzer commented Jan 17, 2022 • edited Loading

Summary

The matching PR for cross compiling the different CPU architectures https://github.com/netlify/live-tunnel/pull/112

github-actions bot commented Jan 17, 2022 • edited Loading

📊 Benchmark results

Package size: 355 MB

ehmicky Jan 17, 2022

Choose a reason for hiding this comment

lukasholzer Jan 17, 2022

Choose a reason for hiding this comment

ehmicky Jan 17, 2022 • edited Loading

Choose a reason for hiding this comment

ehmicky Jan 18, 2022

Choose a reason for hiding this comment

lukasholzer Jan 19, 2022

Choose a reason for hiding this comment

erezrokah left a comment

Choose a reason for hiding this comment

ehmicky commented Jan 18, 2022

lukasholzer commented Jan 18, 2022

lukasholzer commented Jan 18, 2022

lukasholzer commented Jan 18, 2022

charliegerard commented Jan 18, 2022

lukasholzer commented Jan 17, 2022 •

edited

Loading

github-actions bot commented Jan 17, 2022 •

edited

Loading

ehmicky Jan 17, 2022 •

edited

Loading