-
-
Notifications
You must be signed in to change notification settings - Fork 8
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
[download] Migrate to Pulsar's Rolling Release Repo, off of CirrusCI #117
Conversation
In my ideal design in my head, the download microservice would speak both Cirrus CI and GitHub Releases. It could branch its logic based on "ARM Linux/Apple Silicon macOS? --> get from Cirrus", "other stuff? --> get from GitHub Releases rolling". So that the download microservice still works for resolving Apple Silicon and ARM Linux payload URLs. Otherwise the service is dropping our ARM builds. (EDIT: And I surely don't want to try and yeet the Cirrus Rolling binaries onto the GitHub Releases Rolling, that's too much shenanigans for me. Although arguably on the same level and degree of shenanigans as having the download microservice speak to two binary hosts with their own JSON APIs, but eh...) As for the PR as-is, I haven't taken a close look, but it's nice if this is working already for GitHub Releases. |
@DeeDeeG You do have a really good point there. Although what if we take a slightly different approach and instead add scripts to the end of builds on cirrus to upload those to the github releases? Since you can see in the PR the GitHub API is much simpler to work with, and would be nice to have a proper visual archive of all our rolling releases. Although I do get if that's difficult to do and we have to add back in the capabilities to work with Cirrus. Since that wasn't something fully considered during the drafting of this PR. Sure this PR will still look for ARM and Silicon binaries, but I had forgotten that we had no plans to make them available here |
🤷 It's a little bit down to what we can dream up, design and implement in the next few days. Not trivial, theoretically doable. I'm sure we can add back more stuff if some of it gets left on the cutting room floor in order to get something out the door in time. Something is better than nothing, to be sure. Edit: Not to downplay efforts, I think we're doing pretty darn well in response to this situation, as always being volunteers in our spare time. Including this PR seems to have taken some real work to it already. Kudos to all that's working already in this and thanks for this and all the other efforts!! We're gettin' through this. |
Appreciate you! But very true, we are limited by time. Although over on the GitHub Actions PR on the main repo, I put all the logic of uploading artifacts into it's own NodeJS script, not relying on much else, so it may be totally possible to get Cirrus to run that same script after the fact, I'll actually take a try at implementing that soon, since if so it'd be rad to get this all functioning without to many more logic changes lol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some stuff I noticed, most of it is either really minor or just optional nitpicks.
There's one &&
check that I think is meant to be ||
, but other than that, I think this looks pretty good! Worked in my testing already, as-is!
Thank you for this!!!!
Posting my test results, as always for this microservice's PR's: My old // This is test-find-link.js
const utils = require("./utils.js");
(async () => {
console.log(await utils.findLink("arm_linux", "linux_deb"));
})(); Test output when modifying and running
|
Co-authored-by: DeeDeeG <[email protected]>
Co-authored-by: DeeDeeG <[email protected]>
@DeeDeeG Addressed everything you commented on, if you'd like to do one more review so we can push this. Apparently our changes to cirrus have in fact already broken the download microservice |
for (const version of releases) { | ||
for (const asset of version.assets) { | ||
|
||
let name = asset.name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let name = asset.name; | |
let name = asset?.name; |
If you want to put optional chaining back, I might have been misinformed about just how "impossible" it is for asset
to be undefined
here (I have found out it is indeed possible, if you loop through an array with literal undefined
in it).
We're pulling data from JSON where undefined
isn't allowed, so it should be safe, but in general I think you had the motivation/implementation probably right before, sorry about that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approve with or without the optional chaining, feedback was addressed and implementation works in my testing, should un-break the download microservice in production.
(EDIT to add: Note on the source of the microservice breakage happening that this PR should fix... We have completed
builds with no tasks now, given the filtering we're doing for what runs can/can't go through on Cirrus... Which the old logic didn't account for! That older stuff was complex and turned out to be a bit brittle. A simpler API to interface is certainly a blessing for this microservice.)
Another good PR, lots of these flying fast around here, kudos for all the hard work.
Thanks a ton @DeeDeeG, appreciate you and all your work on this PR as well. Gonna go ahead and merge, then push to production! |
This PR removes our
download
microservices interaction with CirrusCI, instead now collecting rolling releases frompulsar-edit/pulsar-rolling-releases
repository.These changes relate to the necessary changes needed to be made, detailed in
pulsar-edit/pulsar#685
.And should resolve any and all issues related to the download microservice, to ensure uninterrupted functionality of our download microservice.