-
Notifications
You must be signed in to change notification settings - Fork 234
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
automatic release to GitHub #279
Conversation
in my tests upx brought a reduction to 54% of the original size, which amounted to 4,5MiB decrease. As we are talking about <10MiB here, I removed it to keep the complexity low.
For now I only implemented the dev build. What is missing is the actual releasing mechanics. |
This PR is ready for review. Anyone? |
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.
Had to take a little bit of time to familiarize myself with the Github CICD pipeline and its integration. Looks good to me!
LGTM |
It just dawned on me, that I totally forgot to implement the key handling for the signer script. Will do that later today - latest tomorrow. |
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.
Now, this last commit adds the missing magic:
I created a secret variable in Github, which is decoded (base64 -d
) and put in place. Then make release
is run (which also runs the signer command) and finally I create a md5sum - just for good measures. In the end a release is being created and all those files are attached to it.
@@ -1,5 +1,5 @@ | |||
UPDATE_URL= | |||
UPDATE_PUBKEY= | |||
UPDATE_PUBKEY=Md2rdsS+b6W0trgcqa5lAWP978Zj0sFmubJ252OPKwc= |
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.
This is the public key that corresponds with the new signer key. They are used to in the end let update.go
verify the integrity of our new release artifact.
with: | ||
files: | | ||
${{ github.workspace }}/bin/NoiseTorch_x64_*.tgz | ||
${{ github.workspace }}/bin/NoiseTorch_x64_*.tgz.sig | ||
${{ github.workspace }}/bin/NoiseTorch_x64_*.tgz.md5sum |
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.
md5sum's are more common and more people know how to use them to verify the integrity of files.
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.
Whoops, sorry, meant to do a comment, not a review, so I deleted that. I'm still partial to doing an sha256sum over an md5 or sha1, given the cheapness of those attacks. At the very least, could we include both md5 and sha256?
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.
I would agree, if this was used to secure anything vital. But it's only a checksum to verify if the file has been altered or is another file altogether. You think this is still a reason to use some more sophisticated checksum algorithm?
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.
oh, well. why not? -> see 536f5b3
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.
Thanks! In reality, there probably isn't a huge amount of risk but it's something that doesn't hurt at all to add :)
I appreciate it getting added to the PR.
closes #272