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(attach_debug_apks_to_release): delete release on failure to build/upload #3241

Merged
merged 2 commits into from Feb 15, 2023

Conversation

thunder-coding
Copy link
Member

env:
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
steps:
- name: Delete release (if other job(s) failed)
Copy link
Member

Choose a reason for hiding this comment

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

Don't need (if...). Otherwise, looks fine. Will have to test these changes and verify hashes when I make the release.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it just in case so that anyone just wondering through logs does not wonder why someone would want to delete release on create_release.

Copy link
Member Author

Choose a reason for hiding this comment

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

And about the checksums, they are correct as per my knowledge. I had checked them when moving to our fork for termux/termux-packages' bootstrap archives uploads.

Also I had tested the CI by running it on my fork as a test. https://github.com/thunder-coding/termux-app/releases/tag/v0.118.5 (The test release). https://github.com/thunder-coding/termux-app/actions/runs/4124697065 (Respective CI run)

Copy link
Member

Choose a reason for hiding this comment

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

I added it just in case so that anyone

if: failure() would have made it clear too, but okay.

And about the checksums, they are correct as per my knowledge. I had checked them when moving to our fork for termux/termux-packages' bootstrap archives uploads.

Your checksum files don't follow the standard sha256sum and md5sum formats and so automated validation would fail.

https://superuser.com/questions/1566136/is-the-sha256-file-format-formally-defined-somewhere-how-should-it-be-parsed

Its also adding apt-android-*/ before filenames, which is also wrong, there is no directory structure in the release itself. It would exist because you are first uploading the apks to github artifacts and then downloading them again, which has the apt-android-* artifacts, so again automated validation would fail since files won't be found and users would have to add additional checks for which subdirectory the files are expected to be under.

The old way directly uploaded the apks to release and didn't have any artifacts. Also the uppercase CHECKSUMS looks ugly, ideally it should be something like release.sha256 with a standard extension, old way is not adding the extension either.

I also didn't deliberately add md5sum support previously, since its not really secure and should not be used for security related purposes.

https://en.wikipedia.org/wiki/MD5#Security


https://github.com/termux/termux-app/actions/runs/1669634446

https://github.com/termux/termux-app/releases/tag/v0.118.0

  • sha256sum
3141295849db07a1f4c919ee67a261df723a8ff751b990b87d663a6457b81a2c  termux-app_v0.118.0+github-debug_universal.apk
10b412fa0aca5a4585c367a446f8e63525bdc9a7b8dfce2d3f84c2f148e19e21  termux-app_v0.118.0+github-debug_arm64-v8a.apk
84f1d108c1bfb8aed3c0f61253b778ca1d6dfbb2deec2a7f8d1ab89b2c9c7a52  termux-app_v0.118.0+github-debug_armeabi-v7a.apk
90c1e62bcede3897cf097d179658f08340d5adde84d50bfae7af056d6dd6c32e  termux-app_v0.118.0+github-debug_x86_64.apk
a5cbc3aea8c199c4db5fbc3697ad8e92117d354ada845a03713e000950a8574f  termux-app_v0.118.0+github-debug_x86.apk

https://github.com/thunder-coding/termux-app/actions/runs/4124697065

https://github.com/thunder-coding/termux-app/releases/tag/v0.118.5

  • CHECKSUMS-sha256.txt
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_arm64-v8a.apk	da60a58853f34e8f8d3f6e9ff0c1482a34349cee8362e988d5d66bc58564c87f
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_armeabi-v7a.apk	13c3d506025498b55b59c76cbefd4dca23aaf7db2aef64ab31eabb433230437f
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_universal.apk	7564a28dd1186186d47ae2d04d5a39ba8b84a831d4fb726556aded284958b302
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_x86_64.apk	1883c777ddabfa36457aad88c1e0c572012d71339c9a4dcd6a048dab28957e5d
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_x86.apk	de7bb2fc355de0f6cd183af87e331ef82954ff7d4dabc1ecb38bd44f33ed014b
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_arm64-v8a.apk	c2ee76c4af5c78f7f8132860fdca55c2ff87bf7af2acff1b8017644171094ef9
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_armeabi-v7a.apk	92abcedc592ca59ef0baeb89bae919db771555d434d9328a7bd7dd84eb3701e6
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_universal.apk	95bd25fab12fcb11174d96cd75dccdfc9e04300c3fdc3f416d7a3ac92cd76f58
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_x86_64.apk	b28d7a00341a4811fa5439769ebc5e893cbcf5547ced8c8e8846e3135716c6ed
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_x86.apk	366427cc6b59df76b2ee578b6143efd88fbe28630d645b0e790c4e51b5aa2c75
  • CHECKSUMS-md5.txt
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_arm64-v8a.apk	d69abede860f42994cffd55303290ca1
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_armeabi-v7a.apk	2b424d20e62ba0cc42beb7e0e2ad845c
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_universal.apk	344be1bdf4a6e4302eab8eb85372850e
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_x86_64.apk	11ea381736ba19785a67adf4f4f44122
apt-android-5/termux-app_v0.118.5+apt-android-5-github-debug_x86.apk	caddd2a5b4ae42021d00eb7a35d39b22
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_arm64-v8a.apk	941ed23c678bc045f12ea40b547d30e0
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_armeabi-v7a.apk	159317799c52328c217a2fb0207485b6
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_universal.apk	3ddfaac1fe22339cb1538a7dca231e0f
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_x86_64.apk	4409b42f2852615a5f7911d90d5aedfe
apt-android-7/termux-app_v0.118.5+apt-android-7-github-debug_x86.apk	ffb801db9c43ed8e8fc008f74f4c8b33

@agnostic-apollo
Copy link
Member

And for the record, I also personally don't like the needless use of external actions in 2ac7fd1 to generate and upload checksums which was simply being done with hub and sha256sum native commands. The more external dependencies that are used, the more the attack surface for attacks and vulnerabilities. There is also no advantages that I see, even the lines of code are roughly same.

@thunder-coding
Copy link
Member Author

And for the record, I also personally don't like the needless use of external actions in 2ac7fd1 to generate and upload checksums which was simply being done with hub and sha256sum native commands. The more external dependencies that are used, the more the attack surface for attacks and vulnerabilities. There is also no advantages that I see, even the lines of code are roughly same.

Kinda agree. Also the hub cli is quite dead. There was a brief time since 2020 when there were no commits to the repository. Also the last release is in March 2020. Maybe if not moving to upload-release-actions, we should consider using the GitHub CLI.

Talking about security for upload-release-action, I'm taking security quite seriously for the fork. Dependency updates are done by dependabot only, (even I don't touch the lockfile after migration to yarn). Also doesn't have much runtime dependencies except for those packages owned by github itself (for interacting with the API), and the glob module

@agnostic-apollo
Copy link
Member

Kinda agree. Also the hub cli is quite dead. There was a brief time since 2020 when there were no commits to the repository. Also the last release is in March 2020. Maybe if not moving to upload-release-actions, we should consider using the GitHub CLI.

Why update something that's working just fine as is and is stable? There is no need to endlessly update every software for the "whens-the-next-update-craze", other than security patches. Using cli for other reasons is fine though.

Talking about security for upload-release-action, I'm taking security quite seriously for the fork. Dependency updates are done by dependabot only, (even I don't touch the lockfile after migration to yarn). Also doesn't have much runtime dependencies except for those packages owned by github itself (for interacting with the API), and the glob module

If there are no dependencies to update yourself, then that's often better. But its good that you are taking security seriously. Technically, hub and sha256sum would have their own dependencies, but updates should be managed automatically by github.

https://github.com/github/hub/blob/master/Gemfile

thunder-coding added a commit to termux/upload-release-action that referenced this pull request Feb 15, 2023
BREAKING CHANGE: Previous behaviour was to use the path name as is provided in the glob (after expanding), or simply using the file_name parameter.
This behaviour is inconsistent, since GitHub releases does not have this directory structure (basically it's missing).
Earlier:
```
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx	path/to/file.txt
...
```
Now:
```
xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx	file.txt
...
```

See termux/termux-app#3241 (comment)
@thunder-coding thunder-coding merged commit 1eb0912 into master Feb 15, 2023
@thunder-coding
Copy link
Member Author

I'm so sorry, once again I pushed accidently to termux:master instead of my fork. Recreating this PR. Also fixed the commits by force pushing

@termux termux deleted a comment from astfzaze010 Feb 25, 2023
agnostic-apollo added a commit that referenced this pull request Apr 16, 2024
…ug builds to new releases"

This reverts commit 2ac7fd1.

Do not use `upload-release-action` for uploading artifacts and generating checksum and instead keep using standard `sha256sum` and internal github tools. `upload-release-action` also generates checksum in the wrong format, check #3241 (comment).
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.

None yet

2 participants