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

chore: Bump upload-artifact@v3 to v4 #725

Merged
merged 11 commits into from
Nov 28, 2024

Conversation

sungwy
Copy link
Contributor

@sungwy sungwy commented Nov 27, 2024

@sungwy sungwy requested a review from Xuanwo November 27, 2024 13:44
@sungwy sungwy requested a review from Fokko November 27, 2024 13:50
Copy link

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

is it possible to test this on your action on your fork?

@@ -132,7 +133,7 @@ jobs:

steps:
- name: Download all the dists
uses: actions/download-artifact@v3
uses: actions/download-artifact@v4
with:
name: wheels

Choose a reason for hiding this comment

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

should this also be changed to

        name: wheels-*
        merge-multiple: true

Copy link
Contributor Author

Choose a reason for hiding this comment

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

great catch!

@sungwy
Copy link
Contributor Author

sungwy commented Nov 27, 2024

is it possible to test this on your action on your fork?

I think the easier way would be for me to test it against this repository using the pull_request trigger by temporarily commenting out the testpypi-publish condition. I'm catching a few bugs testing this out :) Thanks for the suggestion!

@sungwy
Copy link
Contributor Author

sungwy commented Nov 27, 2024

@kevinjqliu
Copy link

kevinjqliu commented Nov 27, 2024

nice! I see it at https://test.pypi.org/project/pyiceberg-core/

and I was able to install it locally

poetry run pip install -i https://test.pypi.org/simple/ pyiceberg-core
poetry run python

import pyiceberg_core
dir(pyiceberg_core)

Built Distributions looks correct:
https://test.pypi.org/project/pyiceberg-core/#files

Copy link

@kevinjqliu kevinjqliu left a comment

Choose a reason for hiding this comment

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

thanks for adding this!

@sungwy
Copy link
Contributor Author

sungwy commented Nov 27, 2024

thanks for adding this!

@kevinjqliu Thanks for the review! :)

Copy link
Contributor

@liurenjie1024 liurenjie1024 left a comment

Choose a reason for hiding this comment

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

LGTM, but better waiting for @Xuanwo and @Fokko to take a review since I'm not familiar with python distribution.

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you @sungwy, love it! Also thank you @kevinjqliu and @liurenjie1024 for review.

@Xuanwo Xuanwo merged commit 4e5187b into apache:main Nov 28, 2024
25 checks passed
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.

4 participants