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

Another try to fix CI #109

Merged
merged 1 commit into from
Mar 7, 2022
Merged

Another try to fix CI #109

merged 1 commit into from
Mar 7, 2022

Conversation

Shadowghost
Copy link
Contributor

  • properly generate checksums only for existing files
  • only upload relevant files to release

@crobibero crobibero merged commit 9963c9f into jellyfin:jellyfin Mar 7, 2022
Copy link
Member

@oddstr13 oddstr13 left a comment

Choose a reason for hiding this comment

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

asset_path needs fixing.
The find suggestions are just that, suggestions.

@@ -92,7 +92,7 @@ jobs:
- name: Prepare Release Assets
run: |-
pushd artifact
for file in *.deb *.zip; do
find * -type f \( -name "*.deb" -o -name "*.zip" \) | while read file; do
Copy link
Member

Choose a reason for hiding this comment

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

While I would hate whoever made uppercase bits in these file extensions, it may be worth using -iname for case insensitive filename matching.

There is also a difference between find * and find ., dotfiles are not matched by *.
Not that we'd ever want dotfiles as actual artifacts tho, just pointing it out.

Suggested change
find * -type f \( -name "*.deb" -o -name "*.zip" \) | while read file; do
find * -type f \( -iname "*.deb" -or -iname "*.zip" \) | while read file; do

I prefer the long version of or, as it's more clear on what it does without being familiar with find (but it isn't POSIX, which shouldn't matter as we're running this under Ubuntu).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree on using iname but the use of * is intentional to strip the leading ./ from the result of find.

Comment on lines +105 to +107
./artifact/*.zip'
./artifact/*.deb'
./artifact/*.sha256sum'
Copy link
Member

Choose a reason for hiding this comment

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

Copypasta?

Suggested change
./artifact/*.zip'
./artifact/*.deb'
./artifact/*.sha256sum'
./artifact/*.zip
./artifact/*.deb
./artifact/*.sha256sum

Depending on how the glob works here, it may not pick up files in sub-directories.
I'm not sure if that ever matters here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bad copypasta and originally it was intentional to only grab on one level but I guess it doesn't hurt picking up recursivly.

Fixes didn't work anyway so I'll change it in the next try...

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.

3 participants