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.

Sign up for GitHub

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

Jump to bottom

update manylinux wheels download logic #1250

Merged
merged 12 commits into from
Aug 20, 2023
Merged

update manylinux wheels download logic #1250

merged 12 commits into from
Aug 20, 2023

Conversation

sridhar562345
Copy link
Contributor

@sridhar562345 sridhar562345 commented May 23, 2023 •
edited
Loading

Description

The current manylinux regex is not matching some wheel files. Updated the Manylinux wheels download logic to fix this.

GitHub Issues

#1249
#1063

monkut
monkut requested changes May 24, 2023
View reviewed changes
Copy link
Collaborator

@monkut monkut left a comment

Choose a reason for hiding this comment

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

Please add a testcase to identify and confirm the issue

@coveralls
Copy link

coveralls commented May 24, 2023 •
edited
Loading

Coverage Status

coverage: 74.688% (+0.08%) from 74.605% when pulling 2323a82 on sridhar562345:fix-manylinux-logic into 26d6182 on zappa:master.

@sridhar562345 sridhar562345 requested a review from monkut May 24, 2023 04:53
@sridhar562345
Copy link
Contributor Author

@monkut
Added tests to verify the updated logic

monkut
monkut requested changes Jun 9, 2023
View reviewed changes
zappa/core.py Show resolved Hide resolved
zappa/core.py Show resolved Hide resolved
@monkut
Copy link
Collaborator

monkut commented Jun 9, 2023

@sridhar562345 is this separate from #1244 ?
Or does this include that one? Is so, let's close 1244.

@sridhar562345
Copy link
Contributor Author

@sridhar562345 is this separate from #1244 ? Or does this include that one? Is so, let's close 1244.

This is different from that, but we can close #1244 since it is not supported by lambda yet.

@sridhar562345 sridhar562345 requested a review from monkut June 9, 2023 05:17
monkut
monkut requested changes Jun 22, 2023
View reviewed changes
zappa/core.py Outdated Show resolved Hide resolved
tests/tests.py Outdated Show resolved Hide resolved
monkut
monkut reviewed Jun 22, 2023
View reviewed changes
zappa/core.py Show resolved Hide resolved
@sridhar562345 sridhar562345 requested a review from monkut July 4, 2023 12:27
monkut
monkut requested changes Aug 19, 2023
View reviewed changes
tests/tests.py Outdated Show resolved Hide resolved
@sridhar562345
Copy link
Contributor Author

@monkut thanks for the changes, I have been thinking about discussing this with you.

souravjamwal77
souravjamwal77 approved these changes Aug 20, 2023
View reviewed changes
Copy link
Collaborator

@souravjamwal77 souravjamwal77 left a comment

Choose a reason for hiding this comment

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

Tests ran successfully on my side too, however, due to time constraints I wasn't able to test the feature itself.

@monkut monkut merged commit f2f03ba into zappa:master Aug 20, 2023
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@monkut monkut monkut approved these changes

@souravjamwal77 souravjamwal77 souravjamwal77 approved these changes

Assignees
No one assigned
Labels
next-release-candidate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@sridhar562345 @coveralls @monkut @souravjamwal77 @storkwrangler