src: Stop pinging backend during package uninstalls #103
Conversation
So, we stopped *using* these pings on our implementation of the backend, since it doesn't really make sense to decrement the install count when someone uninstalls... And it may or may not have simpoly been telemetry when the O.G. Atom backend was expecting this to be an authed action. Since we don't need to auth or ping the backend for uninstalls... Just delete the code that does it!
There was a problem hiding this comment.
Easy approval!
Thanks for finally getting caught up on this on, as it's been in the back of my head for far too long.
Any logic was removed from the backend way back in February, so this has certainly been a long time coming.
But for the most part tests look happy, beyond what can only be assumed external factors, so I'd say we are good to merge.
(Plus you were right, this is a really clean removal)
External factors, yes. Apparently we haven't ran CI on this repo in a while, so we weren't hit with the "older node-gyp, newer Python" issues here until now. (GitHub Actions just pulled in the latest ppm's bundled node-gyp is new enough to handle this, but the CI runner's Node 14 --> npm 6 --> node-gyp 5 is too old to work with Python 3.9+. Also, the CI runner's versions of npm --> node-gyp 9/older aren't patched for Python 3.12 yet either. I'll see how we fixed this in other repos, but maybe that should be a separate PR to fix CI, since it's kind of unrelated to this PR, strictly speaking. |
|
Thank you for review, I'll go ahead and merge this, since it's tested working on my machine locally, works in CI (when CI isn't having a "node-gyp version" + "Python version" mutual incompatibility issue), etc. Don't want to forget about this one or have to pick it up again tomorrow when I may have forgotten some of the details. It's fresh in my mind now, and I think it looks good, so here we go, merging. |
| @@ -82,9 +61,6 @@ Delete the installed package(s) from the ~/.pulsar/packages directory.\ | |||
| if (fs.existsSync(packageManifestPath)) { | |||
| const packageVersion = this.getPackageVersion(packageDirectory); | |||
There was a problem hiding this comment.
Whoops, didn't notice packageVersion is defined but now unused.
Didn't notice I could/should delete this line, too. Can do it in a follow-up, I guess.
So, we stopped using these pings on our implementation of the backend, since it doesn't really make sense to decrement the install count when someone uninstalls... arguably. And it may or may not have simply been telemetry when the O.G. Atom backend was expecting this to be an authed action.
Since we don't need to auth or ping the backend for uninstalls... Just delete the code that does it!