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

Update upload-artifact to use v4 #1371

Merged

Conversation

kevinjqliu
Copy link
Contributor

@kevinjqliu kevinjqliu commented Nov 25, 2024

Both actions/upload-artifact@v3 and actions/download-artifact@v3 are deprecated and requires update, based on https://lists.apache.org/thread/nx19j3h61tjmsk8c8vx24gbq6ygf7pyf

upload-artifact@v3 only used in iceberg-python
https://grep.app/search?q=actions/upload-artifact%40v3&filter[repo.pattern][0]=apache/iceberg

download-artifact@v3 is not used in any iceberg-related repos
https://grep.app/search?q=actions/download-artifact&filter[repo.pattern][0]=apache/iceberg

Migrate from V3 to V4 following the migration guide to merge multiple artifacts into one with @actions/upload-artifact/merge.

Removed macos-12 from the os matrix since its deprecated and added macos-15 which is currently in public preview.

Testing "Python Release" action

Downloaded artifacts and verified that they have the same file names, but different hash.

@kevinjqliu kevinjqliu requested a review from Fokko November 25, 2024 20:03
@Fokko
Copy link
Contributor

Fokko commented Nov 25, 2024

Thanks for bumping this, unfortunately, we've downgraded to v3 a while ago #396 for a specific reason.

With V4 it does not combine the zip files, so we get 5 zip files that we need to unpack, and then combine again. With #1370 this would explode to 25 zips. While releasing iceberg-go I noticed that they use a very fancy script that gets the relevant information of the release PR through the GitHub API. This removes quite a few steps and might be a good idea to introduce here when we're going to v4.

@kevinjqliu kevinjqliu force-pushed the kevinjqliu/remove-deprecated-actions branch 2 times, most recently from e55202a to 1d551c2 Compare November 25, 2024 20:50
path: ./wheelhouse/*
merge:
Copy link
Contributor

Choose a reason for hiding this comment

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

@@ -34,7 +34,7 @@ jobs:
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ ubuntu-22.04, windows-2022, macos-12, macos-13, macos-14 ]
os: [ ubuntu-22.04, windows-2022, macos-13, macos-14 ]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed macos-12 because its deprecated and causing the merge step to not run

https://github.com/kevinjqliu/iceberg-python/actions/runs/12018552791

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch, I noticed that macos-15 is also available. Maybe we can add that one as well. I checked it on my local branch, and it works over there: https://github.com/Fokko/iceberg-python/actions/runs/12033072248

@kevinjqliu kevinjqliu requested a review from Fokko November 25, 2024 22:40
@kevinjqliu
Copy link
Contributor Author

@Fokko I double-checked the new artifacts, file names match but they have different hashes.

@Fokko
Copy link
Contributor

Fokko commented Nov 26, 2024

@kevinjqliu Can you elaborate? I just pushed it to my own branch, and I see identical hashes:

➜  Downloads shasum -a 512 release-ubuntu-22.04/pyiceberg-0.8.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl 
097dc7cc8cf214b3489e3c8facf66e08212570ca5f5d0cf16df88cd5b7839e76a2807e1326d80716ddb46bbbe4cdd423599ff3db0265cfd56c79ebe56678e39a  release-ubuntu-22.04/pyiceberg-0.8.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
➜  Downloads shasum -a 512 release-main/pyiceberg-0.8.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl
097dc7cc8cf214b3489e3c8facf66e08212570ca5f5d0cf16df88cd5b7839e76a2807e1326d80716ddb46bbbe4cdd423599ff3db0265cfd56c79ebe56678e39a  release-main/pyiceberg-0.8.0-cp310-cp310-manylinux_2_17_x86_64.manylinux2014_x86_64.whl

@kevinjqliu kevinjqliu force-pushed the kevinjqliu/remove-deprecated-actions branch from 6fb37f2 to 53f8b02 Compare November 26, 2024 17:14
@kevinjqliu
Copy link
Contributor Author

kevinjqliu commented Nov 26, 2024

I was comparing between running the action on main versus this branch

Do you know a better way to verify this?

@Fokko
Copy link
Contributor

Fokko commented Nov 26, 2024

@kevinjqliu Ah I see, I don't think that we have fully reproducible builds. For example, if a timestamp in some wheel is different, then it would yield another hash. I think it is a nice feature to have, but not strickly required for the ASF releases AFAIK.

@kevinjqliu kevinjqliu merged commit 3230186 into apache:main Nov 26, 2024
8 checks passed
@kevinjqliu kevinjqliu deleted the kevinjqliu/remove-deprecated-actions branch November 26, 2024 21:05
This was referenced Nov 27, 2024
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
* use v4

* merge artifacts

* remove mac 12

* remove old artifacts

* add macos-15
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 7, 2024
* use v4

* merge artifacts

* remove mac 12

* remove old artifacts

* add macos-15
sungwy pushed a commit to sungwy/iceberg-python that referenced this pull request Dec 24, 2024
* use v4

* merge artifacts

* remove mac 12

* remove old artifacts

* add macos-15
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