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

CI: Fix tag Linux binaries are uploaded to for Rolling #901

Merged
merged 1 commit into from
Feb 6, 2024

Conversation

DeeDeeG
Copy link
Member

@DeeDeeG DeeDeeG commented Jan 30, 2024

Identify the Bug

Linux binaries are getting uploaded to the wrong tag/release name, such as 1.113.0-dev instead of 1.113.2024013003 over at the pulsar-rolling-release repo, since #858 landed.

Description of the Change

The Rolling binary upload script gets the tag version from package.json in the repository files, not from the binary itself. So, we need to set this value in the repository's package.json file, regardless of what version the binary itself has.

The version string in package.json is used by the Rolling upload script to decide what the tag name will be when creating a new Rolling release. We want timestamped version strings and tag names, so that they are unique and older releases are not overwritten/clobbered/won't have conflicts (whichever would have happened, not worth finding out). Besides that this restores the convention we had been uploading the Rolling release tags with so far.

Set a version timestamp just before building the binaries, like on the other two platforms. Add this to the outputs of the "build" job if on Linux. Then read this output in the "test and upload, Linux" job. Now we have synced timestamps again (as much as we did before building Linux binaries in a Debian 10 Docker container, anyway).

Alternate Designs

The Rolling binary upload script could be updated to check the binary itself, but this way is easier.

Possible Drawbacks

Adds a little more to the workflow definition, which is getting a bit long, might be harder for a new contributor to read, or for existing contributors to catch up on if it's been a bit (especially if they haven't reviewed/seen #858).

Verification Process

No YAML syntax error in CI, CI shouldn't complain running this PR, CI logs should look right.

Easiest to test if it's fully, properly working after landing this, but there should be good indications in CI that it's going according to plan.

EDIT: Please do wait for me to do some verification of how it's looking from what we can see just in this PR. I'll be able to give it a closer look later when I get a free moment to do so. That'll give confidence this is going the right way and save the hassle of hotfixing it again if it needed it.

EDIT 2: Seems okay, see: #901 (comment).

Release Notes

Fix the tag Linux binaries are uploaded to at the Rolling releases repo

The Rolling binary upload script gets the version from
package.json in the repository files, not from the binary
itself. So, we need to set this value in the repository's
package.json file, regardless of what version the binary itself has.

The version string in package.json is used by the Rolling upload script
to decide what the tag name will be when creating a new Rolling release.
We want timestamped version strings so they are unique,
and older releases are not overwritten/clobbered/won't have conflicts
(whichever would have happened, not worth finding out).
Besides that this restores the convention we had been uploading
the Rolling release tags with so far.

Set a version timestamp just before building the binaries, like on
the other two platforms. Add this to the outputs of the "build" job
if on Linux. Then read this output in the "test and upload, Linux" job.
Now we have synced timestamps again (as much as we did before building
Linux binaries in a Debian 10 Docker container, anyway).

The script could be updated to check the binary itself,
but this way is easier.
@DeeDeeG DeeDeeG marked this pull request as draft January 30, 2024 06:09
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Feb 6, 2024

This in the CI logs looks good to me: https://github.com/pulsar-edit/pulsar/actions/runs/7706523958/job/21002794876#step:7:1

> Check Pulsar Version
Run sed -i -e "s/[0-9]*-dev/${TIMESTAMP}/g" package.json
  sed -i -e "s/[0-9]*-dev/${TIMESTAMP}/g" package.json
  shell: /usr/bin/bash -e {0}
  env:
    GITHUB_TOKEN: ***
    PYTHON_VERSION: 3.12
    NODE_VERSION: 16
    ROLLING_UPLOAD_TOKEN: ***
    RUN_WINDOWS_VT: false
    RUN_LINUX_VT: true
    RUN_MACOS_VT: true
    TIMESTAMP: 2024013005

TIMESTAMP env var is passed in from outputs of the main build job. test-and-upload-Linux job should set the version just fine with this step.

So, this looks good to me, ready for review.

@DeeDeeG DeeDeeG marked this pull request as ready for review February 6, 2024 05:15
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Feb 6, 2024

FYI: With #903 merged, we aren't uploading new Linux Rolling bins until this PR right here is merged.

This and #903 are very much complementary, IMO.

Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

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

Overall, looks good to me. And if we have successful CI runs with this workflow to point to, I'm all for merging it.

@DeeDeeG
Copy link
Member Author

DeeDeeG commented Feb 6, 2024

So, we don't have an end-to-end demonstration of this working all the way through, since the Rolling upload script was right to not to try and upload for a PR.

But it looks pretty strong that it's set up to upload to the right tag. Specifically: The second job in the workflow file is receiving the info it needs from the first job in the workflow file. Only way I'd think it could not work still is if I haven't done parameter expansion syntax right in the line here: https://github.com/pulsar-edit/pulsar/pull/901/files#diff-5c3fa597431eda03ac3339ae6bf7f05e1a50d6fc7333679ec38e21b337cb6721R271

Looks right to me, works in my terminal:

" . . . ${TIMESTAMP} . . . "

% export TIMESTAMP=`date -u +%Y%m%d%H`
% sed -i -e "s/[0-9]*-dev/${TIMESTAMP}/g" package.json
% git diff
diff --git a/package.json b/package.json
[ . . . ]
-  "version": "1.113.0-dev",
+  "version": "1.113.2024020618",
[ . . . ]

tl;dr it's about as "confirmed working" as I can get without having it be about to upload an actual Rolling binary out of this PR, which I'd like not to do.


P.S. Hmmm... I could edit the upload script to print "was about to upload but no-op-ing to demonstrate the PR", that would really prove the point... BRB doing that. Why not?

@DeeDeeG DeeDeeG marked this pull request as draft February 6, 2024 18:19
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Feb 6, 2024

Nice, it worked.

From the Rolling upload script (modified to just print the version string and exit):

Version string from package.json is: 1.113.2024020618

(I expect it would have said 1.113.0-dev without this PR. Now it says 1.113.2024020618.)

https://github.com/pulsar-edit/pulsar/actions/runs/7804283537/job/21286922508#step:8:34

Reverting those quick test changes and taking this back out of Draft, actually really looks good to me + confirmed.

@DeeDeeG DeeDeeG force-pushed the fix-Rolling-upload-tag-for-Linux-binaries branch from 29c1fb7 to c58bc5f Compare February 6, 2024 19:15
@DeeDeeG DeeDeeG marked this pull request as ready for review February 6, 2024 19:15
@DeeDeeG
Copy link
Member Author

DeeDeeG commented Feb 6, 2024

I'm gonna take that approve at this point, thank you very much!

Rolling binary uploads for Linux are back to normal!

@DeeDeeG DeeDeeG merged commit d1bb4c2 into master Feb 6, 2024
108 of 109 checks passed
@DeeDeeG DeeDeeG deleted the fix-Rolling-upload-tag-for-Linux-binaries branch April 19, 2024 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants