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

fix: name filter in images.list() #468

Merged
merged 5 commits into from
Nov 25, 2024

Conversation

Mr-Sunglasses
Copy link
Contributor

fix: #457

@rhatdan
Copy link
Member

rhatdan commented Nov 18, 2024

@umohnani8 @jwhonce @inknos PTAL

@inknos inknos self-requested a review November 19, 2024 14:35
Copy link
Contributor

@inknos inknos left a comment

Choose a reason for hiding this comment

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

hey @Mr-Sunglasses thanks for the PR. it looks good although there are a few things that the CI is complaining about, I can take a look into these as they seem unrelated.

The PR is good. Would you like to write some tests for filter?

You can take inspiration from these prs #462 #460 and if you have questions I can help you if you need

Signed-off-by: Kanishk Pachauri <[email protected]>
Signed-off-by: Kanishk Pachauri <[email protected]>
Signed-off-by: Kanishk Pachauri <[email protected]>
Signed-off-by: Kanishk Pachauri <[email protected]>
@Mr-Sunglasses
Copy link
Contributor Author

hey @Mr-Sunglasses thanks for the PR. it looks good although there are a few things that the CI is complaining about, I can take a look into these as they seem unrelated.

The PR is good. Would you like to write some tests for filter?

You can take inspiration from these prs #462 #460 and if you have questions I can help you if you need

Thanks a lot @inknos for the suggestions, I've added the tests for the filter and they are passing.

Also, I investigate on why the test's are failing, the reason seems for the test's failing, The code is not compatible with Python 3.6 , I think we should drop the support of python 3.6 as I no longer support by Python Team IMO we should drop 3.6, 3.7, 3.8 ( Ref. Python unsupported-versions I'm looking forward to your views on this topic.

Thanks again, Looking forward to contribute more to the project!

@Mr-Sunglasses
Copy link
Contributor Author

hey @Mr-Sunglasses thanks for the PR. it looks good although there are a few things that the CI is complaining about, I can take a look into these as they seem unrelated.

The PR is good. Would you like to write some tests for filter?

You can take inspiration from these prs #462 #460 and if you have questions I can help you if you need

Thanks a lot @inknos for the suggestions, I've added the tests for the filter and they are passing.

Also, I investigate on why the test's are failing, the reason seems for the test's failing, The code is not compatible with Python 3.6 , I think we should drop the support of python 3.6 as I no longer support by Python Team IMO we should drop 3.6, 3.7, 3.8 ( Ref. Python unsupported-versions I'm looking forward to your views on this topic.

Thanks again, Looking forward to contribute more to the project!

cc. @rhatdan @inknos

@inknos
Copy link
Contributor

inknos commented Nov 22, 2024

whops, I missed your first update, sorry.

Also, I investigate on why the test's are failing, the reason seems for the test's failing, The code is not compatible with Python 3.6 , I think we should drop the support of python 3.6 as I no longer support by Python Team IMO we should drop 3.6, 3.7, 3.8 ( Ref. Python unsupported-versions I'm looking forward to your views on this topic.

Agree. it's been a topic for a while, but I think the times are mature now and we can drop these versions from upstream

Let's proceed this way: I will make a pr to drop python and fix the tests, then we can merge this and also I'll bundle your changes in 5.3.0

@inknos
Copy link
Contributor

inknos commented Nov 25, 2024

@jwhonce this PR has test issues that are unrelated to the code. This PR is dropping py<3.8 and bumping the release to 5.3.0, and it's a fix.

However, I'd like to see this merged first to include it in the release. Are you ok with me merging, bypassing protections?

@inknos
Copy link
Contributor

inknos commented Nov 25, 2024

/lgtm
/approve

Copy link
Contributor

openshift-ci bot commented Nov 25, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: inknos, Mr-Sunglasses

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 4c1490f into containers:main Nov 25, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] images.list() filter does not work properly
3 participants